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

address no-shape warnings in GaussianFlux

    XMLWordPrintable

Details

    Description

      GaussianFlux relies on the shape slot, and puts noisy warnings in the logs when the shape slot fails.

      However, we probably don't want to add a new flag for GaussianFlux to indicate this failure mode, because it'd be entirely redundant with the shape slot flag. We should figure out some other way to squash this warning - how we do that may depend on whether this is addressed before or after the C++ redesign.

      We should also consider having GaussianFlux add an alias to the schema to point back at the shape slot flag, creating what looks like a specific flag for this failure while actually just being a link back to the shape slot flag. That's probably not worth doing within the current C++ interface, however, as it'd require some unpleasant mucking around with ResultMappers.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch added a comment -

            Perry, review for you; I you to take a look at this because it touches some stuff you've been paying more attention to than I have (how the Input objects behave).

            This includes some cleanups for GaussianFlux, but it's mostly about changing the warning log message that produces when the shape slot fails into a properly-recorded flag. Here's how I did it:

            • When we create an Input, we first check whether any of its dependencies failed. Right now, that can only happen with shapes, because with centroids we can always fall back to the Peak. If that dependency failed, we don't even run the current plugin - we just call fail() with no exception to set the general failure flag.
            • When we register a new wrapped plugin that has dependencies on a slot (as defined by what Input it uses), we add aliases to the Schema that link flags within that plugin to the shape/centroid slot flags. This makes it appear as if a specific failure flag was set on the dependent algorithm, but it's just a reference to the slot flag it depends on. Because these aliases are actually aliases to other aliases, you'll need to build against u/jbosch/DM-1282 of afw (also in review) to get the tests to pass.

            Here's the diff summary. All changes on meas_base u/jbosch/DM-1332:

            meas_base:u/jbosch/DM-1332 % git diff --stat master...u/jbosch/DM-1332
             include/lsst/meas/base/GaussianFlux.h      |   21 ++--------
             include/lsst/meas/base/Inputs.h            |    6 +++
             python/lsst/meas/base/base.py              |   22 ++++++++++
             python/lsst/meas/base/forcedMeasurement.py |   31 ++++++++++----
             python/lsst/meas/base/sfm.py               |   28 +++++++++----
             src/GaussianFlux.cc                        |   45 ++------------------
             tests/testGaussianFluxBasic.py             |   63 +++++++++++++---------------
             7 files changed, 104 insertions(+), 112 deletions(-)

            jbosch Jim Bosch added a comment - Perry, review for you; I you to take a look at this because it touches some stuff you've been paying more attention to than I have (how the Input objects behave). This includes some cleanups for GaussianFlux, but it's mostly about changing the warning log message that produces when the shape slot fails into a properly-recorded flag. Here's how I did it: When we create an Input, we first check whether any of its dependencies failed. Right now, that can only happen with shapes, because with centroids we can always fall back to the Peak. If that dependency failed, we don't even run the current plugin - we just call fail() with no exception to set the general failure flag. When we register a new wrapped plugin that has dependencies on a slot (as defined by what Input it uses), we add aliases to the Schema that link flags within that plugin to the shape/centroid slot flags. This makes it appear as if a specific failure flag was set on the dependent algorithm, but it's just a reference to the slot flag it depends on. Because these aliases are actually aliases to other aliases, you'll need to build against u/jbosch/ DM-1282 of afw (also in review) to get the tests to pass. Here's the diff summary. All changes on meas_base u/jbosch/ DM-1332 : meas_base:u/jbosch/DM-1332 % git diff --stat master...u/jbosch/DM-1332 include/lsst/meas/base/GaussianFlux.h | 21 ++-------- include/lsst/meas/base/Inputs.h | 6 +++ python/lsst/meas/base/base.py | 22 ++++++++++ python/lsst/meas/base/forcedMeasurement.py | 31 ++++++++++---- python/lsst/meas/base/sfm.py | 28 +++++++++---- src/GaussianFlux.cc | 45 ++------------------ tests/testGaussianFluxBasic.py | 63 +++++++++++++--------------- 7 files changed, 104 insertions(+), 112 deletions(-)
            pgee Perry Gee added a comment -

            Let's make this a two part review. This first stage is to understand why you chose to solve the problem the way you did. The second will be any line-by-line code review, though I kind of doubt we will need any.

            When I did the implementation of the GaussianFlux slot usage, I didn't particularly like the way I did it. If you recall, I wanted to give the algorithms more information about the values of the slots they depend on. My concern about the Input classes is that they did not have access to enough information, in general – for example, the errors and the flags other than the general failure flag.

            While the change you made puts the shapeFlag checking in a different place, and makes such checking a general case rather than specific to this algorithm (which I guess is a good idea), it also really reduces the amount of information the algorithm itself needs to see about the shape algorithm. So my question is: is it enough for an algorithm to declare its dependence on a shape slot (through the FootprintCentroidShapeInput) and then have access to the Quadrupole value, or are some algorithms going to want yet more information from the Shape slot which our Input Class does not provide. And what about other Input types?

            pgee Perry Gee added a comment - Let's make this a two part review. This first stage is to understand why you chose to solve the problem the way you did. The second will be any line-by-line code review, though I kind of doubt we will need any. When I did the implementation of the GaussianFlux slot usage, I didn't particularly like the way I did it. If you recall, I wanted to give the algorithms more information about the values of the slots they depend on. My concern about the Input classes is that they did not have access to enough information, in general – for example, the errors and the flags other than the general failure flag. While the change you made puts the shapeFlag checking in a different place, and makes such checking a general case rather than specific to this algorithm (which I guess is a good idea), it also really reduces the amount of information the algorithm itself needs to see about the shape algorithm. So my question is: is it enough for an algorithm to declare its dependence on a shape slot (through the FootprintCentroidShapeInput) and then have access to the Quadrupole value, or are some algorithms going to want yet more information from the Shape slot which our Input Class does not provide. And what about other Input types?
            jbosch Jim Bosch added a comment -

            So my question is: is it enough for an algorithm to declare its dependence on a shape slot (through the FootprintCentroidShapeInput) and then have access to the Quadrupole value, or are some algorithms going to want yet more information from the Shape slot which our Input Class does not provide?

            I think just providing the Quadrupole is sufficient for all current algorithms, but no, I don't think it's something we should be assuming for all future algorithms. While we could address that by adding more Input types, or adding complexity to the Input types we have, I'd rather just wait until the C++ redesign. If that goes in the direction we've been planning, the vast majority of algorithms will be able to just use the new Input-like objects there to get just the centroid or just the shape, with flag checking done for them transparently, and any algorithms that want more control over the error checking or access to uncertainties can access the slot values directly from the record object.

            And what about other Input types?

            I think we're in largely the same situation for other input types - our current mechanism is sufficient for all current algorithms, but it represents a restriction we should not be placing on future algorithms. And in the C++ redesign, we'll be removing that as a restriction, while keeping limited access as a convenience.

            jbosch Jim Bosch added a comment - So my question is: is it enough for an algorithm to declare its dependence on a shape slot (through the FootprintCentroidShapeInput) and then have access to the Quadrupole value, or are some algorithms going to want yet more information from the Shape slot which our Input Class does not provide? I think just providing the Quadrupole is sufficient for all current algorithms, but no, I don't think it's something we should be assuming for all future algorithms. While we could address that by adding more Input types, or adding complexity to the Input types we have, I'd rather just wait until the C++ redesign. If that goes in the direction we've been planning, the vast majority of algorithms will be able to just use the new Input-like objects there to get just the centroid or just the shape, with flag checking done for them transparently, and any algorithms that want more control over the error checking or access to uncertainties can access the slot values directly from the record object. And what about other Input types? I think we're in largely the same situation for other input types - our current mechanism is sufficient for all current algorithms, but it represents a restriction we should not be placing on future algorithms. And in the C++ redesign, we'll be removing that as a restriction, while keeping limited access as a convenience.
            pgee Perry Gee added a comment -

            I am willing to wait, as it would be wasted effort now to worry about either the Inputs or the Slots. But I don't understand why we don't just make the 3 slot types we have more general in terms of errors and flags. It doesn't seem difficult.

            I much prefer that the algorithms have access to the flags over having the flags checked for them. Which is really why this issue comes up on this ticket rather than later. I feel like the Dependency check you have is limited, and the ability to get both error and flag information from the slots is a long term need. So before we go ahead with the design you have in this ticket, I would like to be convinced that a dependency check is all we really need. If that is the case, the way you have implemented it is just a matter of taste, I think.

            Perhaps we should talk on the phone about this.

            pgee Perry Gee added a comment - I am willing to wait, as it would be wasted effort now to worry about either the Inputs or the Slots. But I don't understand why we don't just make the 3 slot types we have more general in terms of errors and flags. It doesn't seem difficult. I much prefer that the algorithms have access to the flags over having the flags checked for them. Which is really why this issue comes up on this ticket rather than later. I feel like the Dependency check you have is limited, and the ability to get both error and flag information from the slots is a long term need. So before we go ahead with the design you have in this ticket, I would like to be convinced that a dependency check is all we really need. If that is the case, the way you have implemented it is just a matter of taste, I think. Perhaps we should talk on the phone about this.
            jbosch Jim Bosch added a comment -

            I much prefer that the algorithms have access to the flags over having the flags checked for them. Which is really why this issue comes up on this ticket rather than later.

            I think this is where we disagree. If the flags are checked for them, we get consistent error handling that's implemented in only one place. If they have to check the flags themselves, then it's up to algorithm writers to handle them in a consistent way, and we have a lot of code duplication.

            That said, I don't want to force algorithm writers to do it that way. I just want to encourage them to do it that way unless they have a reason not to. That's what we have right now - if you don't like the Input/Algorithm interface, write a Plugin yourself - and of course it will get much, much easier with the C++ redesign. In the meantime, the only algorithm we have that uses the shape slot is GaussianFlux, and for it, I think the dependency check is indeed all we need.

            In the case where the slot algorithm doesn't fail completely but has a suspect result, I think things do get more interesting, but I'd also like to defer that problem until we actually have a "suspect" flag (and I think by then, we'll have the C++ redesign in place anyway and this will be a moot point).

            jbosch Jim Bosch added a comment - I much prefer that the algorithms have access to the flags over having the flags checked for them. Which is really why this issue comes up on this ticket rather than later. I think this is where we disagree. If the flags are checked for them, we get consistent error handling that's implemented in only one place. If they have to check the flags themselves, then it's up to algorithm writers to handle them in a consistent way, and we have a lot of code duplication. That said, I don't want to force algorithm writers to do it that way. I just want to encourage them to do it that way unless they have a reason not to. That's what we have right now - if you don't like the Input/Algorithm interface, write a Plugin yourself - and of course it will get much, much easier with the C++ redesign. In the meantime, the only algorithm we have that uses the shape slot is GaussianFlux, and for it, I think the dependency check is indeed all we need. In the case where the slot algorithm doesn't fail completely but has a suspect result, I think things do get more interesting, but I'd also like to defer that problem until we actually have a "suspect" flag (and I think by then, we'll have the C++ redesign in place anyway and this will be a moot point).
            pgee Perry Gee added a comment -

            Let me respond to this comment, and then we can HipChat if you have time.

            It is fine for me that we provide them code to check the flags. I would have put that code inline, in a superclass call like CheckShapeSlot. Why do it that way? Because it will be sitting at the top of example apply code in plain sight, and the implementer will instantly know if the check is doing something they don't want. By hiding the check away, it forces the implementer to go figure out how the Input specializations work.

            I agree that what you have put in works for the current algorithms. But I think the the design is not general enough. It would be a good thing if the patterns we establish work in almost every instance where someone is implementing a custom algorithm. So I think that putting more information in the slot getters is a good way to go. We don't need errors for our current algorithms, but we probably should be propagating them in a well written algorithm. If someone is dependent on the Flux, Centroid, or Shape values, they are probably also dependent on the errors. That said, I don't feel the need to add those things right now. I just feel like adding the Input dependency to solve this one problem is the wrong way to go.

            pgee Perry Gee added a comment - Let me respond to this comment, and then we can HipChat if you have time. It is fine for me that we provide them code to check the flags. I would have put that code inline, in a superclass call like CheckShapeSlot. Why do it that way? Because it will be sitting at the top of example apply code in plain sight, and the implementer will instantly know if the check is doing something they don't want. By hiding the check away, it forces the implementer to go figure out how the Input specializations work. I agree that what you have put in works for the current algorithms. But I think the the design is not general enough. It would be a good thing if the patterns we establish work in almost every instance where someone is implementing a custom algorithm. So I think that putting more information in the slot getters is a good way to go. We don't need errors for our current algorithms, but we probably should be propagating them in a well written algorithm. If someone is dependent on the Flux, Centroid, or Shape values, they are probably also dependent on the errors. That said, I don't feel the need to add those things right now. I just feel like adding the Input dependency to solve this one problem is the wrong way to go.
            pgee Perry Gee added a comment -

            The code looks fine. I ran whole ccd tests with Sdss demo and LsstSim data, and also checked the GaussianFlux badShape and badCentroid flags to be sure that they were set properly.

            My reservations about this checkin I have already expressed to Jim

            (1) Do we really need the "dependency" construction on the Input objects, which seems unnecessary to me. He agreed to revisit doing this check in the Algorithm next Sprint.

            (2) Is it really necessary to use an alias to point badCentroid and badShape flags at the original centroid and shape failure flags. Seems just as easy (and less fragile) to copy these flags in the algorithm code.

            pgee Perry Gee added a comment - The code looks fine. I ran whole ccd tests with Sdss demo and LsstSim data, and also checked the GaussianFlux badShape and badCentroid flags to be sure that they were set properly. My reservations about this checkin I have already expressed to Jim (1) Do we really need the "dependency" construction on the Input objects, which seems unnecessary to me. He agreed to revisit doing this check in the Algorithm next Sprint. (2) Is it really necessary to use an alias to point badCentroid and badShape flags at the original centroid and shape failure flags. Seems just as easy (and less fragile) to copy these flags in the algorithm code.
            jbosch Jim Bosch added a comment -

            Merged to master. As Perry and I have discussed off-line, his concerns are important but are best addressed in the upcoming C++ redesign.

            jbosch Jim Bosch added a comment - Merged to master. As Perry and I have discussed off-line, his concerns are important but are best addressed in the upcoming C++ redesign.

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Perry Gee
              Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.