Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1015

convert GaussianFlux to use shape, centroid slots

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:

      Description

      We should cleanup and simplify the GaussianFlux algorithm to simply use the shape and centroid slot values instead of either computing its own or having configurable field names for where to look these up.

        Attachments

          Issue Links

            Activity

            Hide
            pgee Perry Gee added a comment -

            I want to be sure that I understand this issue. You would like to remove the code which calls getGaussianFlux, and simply do what is currently done in the "fixed" case: i.e., get the Centroid and Shape slots and call getFixedMomentsFlux.

            And I assume that the slots are presumed to be set up prior to the construction of Inputs to the algorithm.

            Show
            pgee Perry Gee added a comment - I want to be sure that I understand this issue. You would like to remove the code which calls getGaussianFlux, and simply do what is currently done in the "fixed" case: i.e., get the Centroid and Shape slots and call getFixedMomentsFlux. And I assume that the slots are presumed to be set up prior to the construction of Inputs to the algorithm.
            Hide
            pgee Perry Gee added a comment -

            Simon, do you have time to review this issue? It is pretty simple and straightforward.

            Please note that GaussianFlux now requires the shape slot to be setup before it can be run, which was not the case previously. However, the error handling when the Shape slot is not defined is not fatal. I will put a comment on this ticket to describe the work to be done.

            Show
            pgee Perry Gee added a comment - Simon, do you have time to review this issue? It is pretty simple and straightforward. Please note that GaussianFlux now requires the shape slot to be setup before it can be run, which was not the case previously. However, the error handling when the Shape slot is not defined is not fatal. I will put a comment on this ticket to describe the work to be done.
            Hide
            pgee Perry Gee added a comment -

            When the shape slot is not setup, the creation of the Input object for the base_GaussianFlux plugins fails prior to the call to its measure() routine. To make error handleable, I had to switch two lines of code in sfm.py. This now produces the LogicError: key is not valid, but does not cause the task to completely fail, as it should. That will be corrected in DM-461, which I am now working on.

            pgeepc2:/sandbox2/pgee/mylsst7/Linux64/mb12> git diff --stat master
            include/lsst/meas/base/GaussianFlux.h | 17 ++++-------
            include/lsst/meas/base/Inputs.h | 8 ++++--
            python/lsst/meas/base/baseLib.i | 2 +-
            python/lsst/meas/base/sfm.py | 2 +-
            src/GaussianFlux.cc | 98 ++++++++++-----------------------------------------------------
            tests/testGaussianFluxBasic.py | 5 ++--
            6 files changed, 29 insertions, 103 deletions

            Show
            pgee Perry Gee added a comment - When the shape slot is not setup, the creation of the Input object for the base_GaussianFlux plugins fails prior to the call to its measure() routine. To make error handleable, I had to switch two lines of code in sfm.py. This now produces the LogicError: key is not valid, but does not cause the task to completely fail, as it should. That will be corrected in DM-461 , which I am now working on. pgeepc2:/sandbox2/pgee/mylsst7/Linux64/mb12> git diff --stat master include/lsst/meas/base/GaussianFlux.h | 17 ++++------- include/lsst/meas/base/Inputs.h | 8 ++++-- python/lsst/meas/base/baseLib.i | 2 +- python/lsst/meas/base/sfm.py | 2 +- src/GaussianFlux.cc | 98 ++++++++++----------------------------------------------------- tests/testGaussianFluxBasic.py | 5 ++-- 6 files changed, 29 insertions , 103 deletions
            Hide
            pgee Perry Gee added a comment -

            Sorry, one more thing. Code is in meas_base, branch u/pgee/DM-1015

            Show
            pgee Perry Gee added a comment - Sorry, one more thing. Code is in meas_base, branch u/pgee/ DM-1015
            Hide
            pgee Perry Gee added a comment -

            This is the diff based on the fixed and rebased version of the branch. It is ready for review.

            include/lsst/meas/base/GaussianFlux.h | 17 ++++--------
            include/lsst/meas/base/Inputs.h | 8 +++---
            python/lsst/meas/base/baseLib.i | 2 +-
            python/lsst/meas/base/sfm.py | 2 +-
            src/GaussianFlux.cc | 135 ++++++++++++
            tests/testGaussianFluxBasic.py | 12 ++++-----
            6 files changed, 37 insertions, 139 deletions
            pgeepc2:/sandbox2/pgee/mylsst7/Linux64/mb13>

            Show
            pgee Perry Gee added a comment - This is the diff based on the fixed and rebased version of the branch. It is ready for review. include/lsst/meas/base/GaussianFlux.h | 17 ++++-------- include/lsst/meas/base/Inputs.h | 8 +++--- python/lsst/meas/base/baseLib.i | 2 +- python/lsst/meas/base/sfm.py | 2 +- src/GaussianFlux.cc | 135 ++++++++++++ tests/testGaussianFluxBasic.py | 12 ++++----- 6 files changed, 37 insertions , 139 deletions pgeepc2:/sandbox2/pgee/mylsst7/Linux64/mb13>
            Hide
            krughoff Simon Krughoff added a comment -

            Did this end up as a pull request on GitHub?

            Show
            krughoff Simon Krughoff added a comment - Did this end up as a pull request on GitHub?
            Hide
            krughoff Simon Krughoff added a comment -

            Review comments:

            include/lsst/meas/base/GaussianFlux.h
            Line 40 & 41: Is it possible to fix this documentation

            O.K. Assuming you are running a branch build on buildbot before merging, this looks fine to me.

            Show
            krughoff Simon Krughoff added a comment - Review comments: include/lsst/meas/base/GaussianFlux.h Line 40 & 41: Is it possible to fix this documentation O.K. Assuming you are running a branch build on buildbot before merging, this looks fine to me.
            Hide
            pgee Perry Gee added a comment -

            There are tickets in to fix these documentation issues.

            Show
            pgee Perry Gee added a comment - There are tickets in to fix these documentation issues.

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Simon Krughoff
              Watchers:
              Jim Bosch, Perry Gee, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.