Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-240 meas_base plugins for CModel magnitudes
  3. DM-1225

add RadialProfile class to manage multi-Gaussian Sersic approximations

    XMLWordPrintable

    Details

    • 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.

        Attachments

          Activity

          Hide
          jbosch 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(-)

          Show
          jbosch 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
          krughoff 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
          krughoff 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
          krughoff Simon Krughoff added a comment -

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

          Show
          krughoff Simon Krughoff added a comment - Sorry, I see it now, it's on shapelet.
          Hide
          krughoff 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.

          Show
          krughoff 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
          jbosch Jim Bosch added a comment -

          include/lsst/shapelet/RadialProfile.h:
          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.

          src/RadialProfile.cc
          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
          jbosch Jim Bosch added a comment - include/lsst/shapelet/RadialProfile.h: 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. src/RadialProfile.cc 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.
          Hide
          jbosch Jim Bosch added a comment -

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

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

            People

            Assignee:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Simon Krughoff
            Watchers:
            Jim Bosch, Simon Krughoff
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.