Review for DM-2195 (Port safe coadd clipping from HSC) #26
Conversation
| @@ -33,7 +33,6 @@ | |||
| #include "lsst/afw/image/Exposure.h" | |||
| #include "lsst/meas/base/Algorithm.h" | |||
| #include "lsst/meas/base/FlagHandler.h" | |||
There was a problem hiding this comment.
FlagHandler.h is now unused.
| @@ -294,6 +302,31 @@ def testPixelFlags(self): | |||
| source, | |||
| exp, | |||
| ) | |||
| # Test that if there is no center and centroider that the object should look at the footprint | |||
| plugin, cat = makePluginAndCat(measBase.PixelFlagsAlgorithm, "test", control) | |||
There was a problem hiding this comment.
In fact there are three tests in quick succession here, and the first one tests what happens if there's no Footprint!
Since you're following a very similar pattern repeatedly (make record, attempt to measure, check for failure), I wonder if this should actually be a series of separate tests calling a common implementation?
Also: a few lines above there's a comment (which GitHub won't let me annotate, because it's not been changed) to the effect that "the new code in meas_base InputUtilities.py now throws...". Is this code still testing InputUtilities, or is it testing your new code above? Please clarify the comment.
There was a problem hiding this comment.
I disagree with splitting out these tests into separate tests. Within this file, each test represents testing the functionality of an individual plugin. It would be weird to have each plugin have one test, except pixel flags which has 3. And I don't really see what it gains you. Separate tests are to indicate which portion of code fails (or preferably passes). In this case that would indicate the problem is in pixel flags. To gain more specific information, one can simply look at the specific failure within the test.
There was a problem hiding this comment.
My suggestion wasn't that they should be separate tests, but that a bit of refactoring within this method could make it shorter and less repetitive (probably by defining a local function and calling that). I agree that it's not worth spending time thinking about though; what you have is fine.
|
In addition to the line by line comments I added above, there's a more significant point here -- I understand why you have replaced the
On a more minor level, please review the commit messages and make sure they make sense. In particular, the summary of aace6f is "Port of HSC-1166 safe coadd clipping", which is pretty misleading -- it's one small part of that port. It might also be nice to include reference to the specific HSC commits (by SHA1) which you're pulling over. And, being picky: please use consistent markup for adding your own commentary (I'm thinking of Finally, while reviewing this, I re-wrote some of the code to sanity check my comments. It's not a complete, polished up implementation, but it does include many of the changes I've requested above. Give me a shout if you'd like a copy to work from. |
| @@ -94,10 +79,13 @@ class PixelFlagsAlgorithm : public SimpleAlgorithm { | |||
| ) const; | |||
|
|
|||
| private: | |||
|
|
|||
| typedef std::map<std::string, afw::table::Key<afw::table::Flag>> KeyMap; | |||
| typedef afw::image::MaskedImage<float> MaskedImageT; | |||
There was a problem hiding this comment.
You don't need MaskedImageT here. (It's already in the anonymous namespace in the .cc file, which is the only place you need it).
~~~Note~~~ Port of HSC-1166 safe coadd clipping. This commit is in a non-working not fully ported state, as changes from HSC-1202 will supersede some of the needed changes to get the port working. As the functionality will be removed from the next port, it was not worth time getting a fully ported stable code running, to simply delete it.
We recently added the CLIPPED mask plane to the pixel flags, but it was only going into the schema if it was defined. This produced different schemas, depending on whether the mask plane was defined, which can lead to subtle problems. Instead of hard-wiring a flag based on a mask plane that may or may not exist, this change makes the list of mask planes (besides the defaults) configurable. That means that the schema will always be the same (when built with the same configuration). This also allows for easy extension of the pixel flags. The hard-coded flags could now (mostly) be removed, except that would rename them, and they are referred to in other parts of the code that we don't want to track down right now. ~~~UPDATE~~~ During the porting of this code from HSC to LSST some changes had to occur, and as such some of the code was re-factored and cleaned up. While not all of the code is exactly the same, all of the conceptual parts were ported over intact. - nlust@astro.princeton.edu
This is part of trac ticket #2838. It has been modified during the port to work with the current LSST stack, but conceptually the port works the same.
Because the PixelFlags was updated to remove remove the enumeration list, and move away from flagHanderlers the unit test behavior needed to be updated to that it was relevant and still tested he appropriate behavior. A bit documentation also needed updated.
PixelFlags now accepts a user supplied list of mask planes to check when creating corresponding pixel flags in a mess record. This change updates the unit test to check this behavior.
Introduce a flag which can be set to indicate when a paticular record seems to be off the image being measured.
natelust
force-pushed
the
tickets/DM-2915
branch
from
827ed44
to
ad20eaa
Compare
No description provided.