# add RadialProfile class to manage multi-Gaussian Sersic approximations

XMLWordPrintable

#### Details

• Type: Technical task
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Sprint:
Measurement-S14-4
• Team:
Data Release Production

#### Description

This issue transfers a new class from the HSC fork, RadialProfile, back to the LSST side. This is used downstream in meas_multifit in the galaxy-fitting algorithms.

#### Activity

Hide
Jim Bosch added a comment -

This is part of an effort to split DM-240 into more-easily-reviewable chunks: a new class (RadialProfile) in the shapelet package, just transferred over from the HSC fork. It provides C++ access to multi-Gaussian approximations to Sersic profiles that are saved in Python pickle files (via a singleton registry that's populated by Python code). It also helps to organize those profiles and provide a bit more information about them. The changeset also includes an overhaul of some plotting code for visualizing these profiles and their approximations to make use of the new class.

All changes are on branch u/jbosch/DM-1225 of shapelet, on a single commit:

 shapelet:u/jbosch/DM-1225 % git show --stat commit ed23b3ce23ed40f75151ae38562b456a3060cdc6 Author: Jim Bosch  Date: Thu Feb 20 15:33:59 2014 -0500    RadialProfile class to make persisted bases available in C++    include/lsst/shapelet.h | 1 +  include/lsst/shapelet/RadialProfile.h | 131 +++++++++++++++++++++++  python/lsst/shapelet/__init__.py | 1 +  python/lsst/shapelet/shapeletLib.i | 14 +++  python/lsst/shapelet/tractor.py | 176 +++++++++++--------------------  src/RadialProfile.cc | 184 +++++++++++++++++++++++++++++++++  tests/profiles.py | 44 ++++++--  7 files changed, 426 insertions(+), 125 deletions(-)

Show
Jim Bosch added a comment - This is part of an effort to split DM-240 into more-easily-reviewable chunks: a new class ( RadialProfile ) in the shapelet package, just transferred over from the HSC fork. It provides C++ access to multi-Gaussian approximations to Sersic profiles that are saved in Python pickle files (via a singleton registry that's populated by Python code). It also helps to organize those profiles and provide a bit more information about them. The changeset also includes an overhaul of some plotting code for visualizing these profiles and their approximations to make use of the new class. All changes are on branch u/jbosch/ DM-1225 of shapelet, on a single commit: shapelet:u/jbosch/DM-1225 % git show --stat commit ed23b3ce23ed40f75151ae38562b456a3060cdc6 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Thu Feb 20 15:33:59 2014 -0500   RadialProfile class to make persisted bases available in C++   include/lsst/shapelet.h | 1 + include/lsst/shapelet/RadialProfile.h | 131 +++++++++++++++++++++++ python/lsst/shapelet/__init__.py | 1 + python/lsst/shapelet/shapeletLib.i | 14 +++ python/lsst/shapelet/tractor.py | 176 +++++++++++-------------------- src/RadialProfile.cc | 184 +++++++++++++++++++++++++++++++++ tests/profiles.py | 44 ++++++-- 7 files changed, 426 insertions(+), 125 deletions(-)
Hide
Simon Krughoff added a comment -

I'm doing this now, but I don't see the branch or pull request. I'm looking meas_multifit. Is that the right place?

Show
Simon Krughoff added a comment - I'm doing this now, but I don't see the branch or pull request. I'm looking meas_multifit. Is that the right place?
Hide
Simon Krughoff added a comment -

Sorry, I see it now, it's on shapelet.

Show
Simon Krughoff added a comment - Sorry, I see it now, it's on shapelet.
Hide
Simon Krughoff added a comment -

line 4: 2013 -> 2014
line 36: profides -> provides

python/lsst/shapelet/tractor.py
line 40: could use a docstring
line 50: I'm not sure I understand the continue. Are you expecting there to be failures? It seems like if the profile can't be created, you'd want to know.
line 93: parameters are not documented.
line 98: why not do all the profiles including the other Sersics?

line 4: 2013 -> 2014

tests/profiles.py
lines 44 and 45: Commented out code.
line 54: I don't think this tests the moments, but maybe I'm missing something.
line 60: Why not test ser3 and ser5?

Looks good in general.

Show
Simon Krughoff added a comment - review comments: include/lsst/shapelet/RadialProfile.h: line 4: 2013 -> 2014 line 36: profides -> provides python/lsst/shapelet/tractor.py line 40: could use a docstring line 50: I'm not sure I understand the continue. Are you expecting there to be failures? It seems like if the profile can't be created, you'd want to know. line 93: parameters are not documented. line 98: why not do all the profiles including the other Sersics? src/RadialProfile.cc line 4: 2013 -> 2014 tests/profiles.py lines 44 and 45: Commented out code. line 54: I don't think this tests the moments, but maybe I'm missing something. line 60: Why not test ser3 and ser5? Looks good in general.
Hide
Jim Bosch added a comment -

line 4: 2013 -> 2014
line 36: profides -> provides

Fixed.

python/lsst/shapelet/tractor.py
line 40: could use a docstring

Done.

line 50: I'm not sure I understand the continue. Are you expecting there to be failures? It seems like if the profile can't be created, you'd want to know.

Not really, but it'd be possible to add a file for a new kind of profile that matches the regexp and yet doesn't have a corresponding RadialProfile subclass in C++, and I wouldn't want that to cause the entire package to fail to import because of that. It would probably make sense to warn here, though...I'll do that.

line 93: parameters are not documented.

Fixed.

line 98: why not do all the profiles including the other Sersics?

Just because I don't actually use them in any of the high-level code, so I didn't need those numbers. Doesn't hurt to add them, though, and I've done so.

line 4: 2013 -> 2014

Fixed.

tests/profiles.py
lines 44 and 45: Commented out code.

Removed.

line 54: I don't think this tests the moments, but maybe I'm missing something.

The test is the last assertClose() in that test; the lhs of that comparison is the moments radius, it should match the result of getMomentsRadiusFactor(). I'll add a code comment to that effect.

line 60: Why not test ser3 and ser5?

Those (and dev, too) have extremely broad tails, so the crude integral I'm using to compute the moments radius doesn't converge very well. Calling getMomentsRadiusFactor() on those invokes the same code as with the exp profile, so I'm not worried about a coverage hole here.

Show
Hide
Jim Bosch added a comment -

All review changes have been squashed onto the previous commit. Merged to master.

Show
Jim Bosch added a comment - All review changes have been squashed onto the previous commit. Merged to master.

#### People

Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Simon Krughoff
Watchers:
Jim Bosch, Simon Krughoff