Review for DM-1946: HSC backport of Footprint merge code #22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rename 'merge' private member to _merge. Make overlaps() method const. Add emacs file mode tag.
FootprintMerge needs to modify Footprint's bounding box directly, but normally we don't want to give users the ability to modify it, so we replace the non-const reference accessor with a friend statement.
FootprintMerge is only used as a helper class for FootprintMergeList, which has the public interfaces we use. While it could currently be used on its own, future changes will make that harder to maintain, so it seems best to just make it a truly private class.
Prior to this change, adding a new Footprint that connected two previously-separate merges would result in the flags from one of the existing merges being lost. To fix this, we had to remove the SourceMerge struct in and instead have FootprintMerge hold the output SourceRecord itself.
Some of this test code wasn't quite testing what it claimed to be testing, because the arrays being passed in were not all the same size, and zip() silently iterates over the smallest of its arguments. The removal of some of the test utility code in this change is just to make it so we have to worry about that problem in fewer places. Also changed the failure mode to an exception instead of a warning and no-op; seems like a logic error that should be fatal in order to better catch bugs in higher-level code.
Simplifies and speeds up logic in FootprintMergeList by erasing duplicates items immediately.
For backwards compatibility, we're keeping around all the old Footprint constructors, which use the default Peak minimal schema. But we now have a ctor that accepts a custom schema, and a Python accessible way to change the schema. Conflicts: include/lsst/afw/detection/Footprint.h src/detection/Footprint.cc
Many of the operations that created new Footprints from old ones internally used the Footprint default constructor, not the copy constructor, and that meant they weren't transferring the new peaks properly. Conflicts: src/detection/Footprint.cc Note that recent improvements on the Footprint grow and shrink algorithms require a separate commit to ensure the peaks are being transferred and updated properly.
| // Iterate over peaks from the original footprint and add them IF they are contained | ||
| // within the shrunken footprint. | ||
| for (auto peakIter = foot.getPeaks().begin(); peakIter != foot.getPeaks().end(); peakIter++) { | ||
| if (shrunk->contains(geom::Point2I(peakIter->getIx(), peakIter->getIy()))) { |
There was a problem hiding this comment.
In this test, you can just use:
shrunk->contains(peakIter->getI())
Peaks from the original Footprint need to be copied into the new Footprints returned by the growFootprint and shrinkFootprint functions. For the latter, only those Peaks lying within the shrunken Footprint get added. The unittest code was updated to test these changes.
The tests/footprintMergeCatalog.py code made use of meas_algorithms DoubleGaussianPsf to create a psf for a generated image. Since afw does not require meas_algorithms as a build dependency, it cannot be assumed that the latter is set up and available. The psf is now created using afw.detection's GaussianPsf.
Conflicts: tests/footprintMergeCatalog.py
laurenam
force-pushed
the
u/lauren/DM-1946
branch
from
abf9ae0
to
eb85b0c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.