DM-31849: Fix miscellaneous forced photometry bugs. #192
Conversation
This avoids duplicate WCS transforms, which can be slow, and it makes TransformedCentroidFromCoordPlugin actually useful with most photometry plugins.
Forced photometry reference catalogs are expected to remain in their original coordinate systems, and that includes their footprints. Footprints attached to the forced photometry measurement catalog were already being created by transforming the reference catalog ones. This addresses DM-31686.
TallJimbo
force-pushed
the
tickets/DM-31849
branch
from
17672a7
to
4ef9ee6
Compare
| spans = lsst.afw.geom.SpanSet.fromShape(ellipse) | ||
| footprint = lsst.afw.detection.Footprint(spans.clippedTo(bbox), bbox) | ||
| localIntPoint = lsst.geom.Point2I(localPoint) | ||
| assert bbox.contains(localIntPoint), ( |
There was a problem hiding this comment.
I would move this higher. Spans probably should have the int point anyway. No sense doing the other stuff if the point is not in there.
There was a problem hiding this comment.
Given that it's an assert, it really shouldn't be tripping at all, so short-circuit performance shouldn't be a concern here. But I'm happy to move it earlier.
(I added this because prior to this ticket generateMeasCat wasn't guaranteeing this even though it intended to, and this assert helped catch the bug, which resulted in 1280219)
| to which footprints should be attached. | ||
| exposure : `lsst.afw.image.Exposure` | ||
| Image object from which peak values and the PSF are obtained. | ||
| scaling : `int`, optional |
There was a problem hiding this comment.
Did you mean to dilate by this? It does not seemed to be used
There was a problem hiding this comment.
Good catch; I was planning to scale the ellipses before making SpanSets out of them, but completely forgot. Will fix.
src/PixelFlags.cc
Outdated
| throw LSST_EXCEPT(pex::exceptions::RuntimeError, "No footprint present."); | ||
| } | ||
| auto fullSpans = footprint->getSpans(); | ||
| if (!footprint) { |
There was a problem hiding this comment.
This is the wrong condition.
| afw::detection::Footprint const& footprint(*measRecord.getFootprint()); | ||
| footprint.getSpans()->clippedTo(mimage.getBBox())->applyFunctor(func, *(mimage.getMask())); | ||
| auto footprint = measRecord.getFootprint(); | ||
| if (!footprint) { |
There was a problem hiding this comment.
I thought best practice with pointers wasn't to explicitly test against nullptr
There was a problem hiding this comment.
It's possible this is one of those things we wrote down one way in our style guide, and then the rest of the went the opposite way, but if so, I can't find it in our style guide (one way or another) and the most authoritative rest-of-the-world guide I know of says to use this form:
TallJimbo
force-pushed
the
tickets/DM-31849
branch
from
d37404a
to
c8c2bb8
Compare
This extends the fix for DM-31686 to forced photometry with DataFrame reference catalogs.
I think this was previously overriding the derived-class defaults with any base-class defaults for the same fields.
This should have been included on DM-17689, but the code here was on a branch that was in progress at the same time and got merged later. Some (but not all) of this fixup commit then got orphaned on a DM-29729 branch, because the main purpose of that ticket was something that got marked invalid.
Pass a float to a method that takes an int, and you get truncation, not rounding; this led to points not quite on the image being included.
TallJimbo
force-pushed
the
tickets/DM-31849
branch
from
c8c2bb8
to
6e30e66
Compare
No description provided.