Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_base
-
Labels:
-
Story Points:2
-
Epic Link:
-
Sprint:Measurement-S14-4
-
Team:Data Release Production
Description
We've never figured out how to handle wrapping multiple-aperture photometry algorithms. They can't use the existing Result objects - at least not out of the box.
We should try to write a new multiple-aperture photometry algorithm from the ground up, using the old ones on the HSC branch as a guide, but not trying to transfer the old code over. The new one should:
- Have the option of using elliptical apertures (as defined by the shape slot) or circular apertures.
- Have a transition radius at which we switch from the sinc photometry algorithm to the naive algorithm (for performance reasons).
Attachments
Issue Links
Activity
Assigning to Frossie for review, but not really.
Perry, this now ready for you to review.
ApertureFlux.cc
typo in line 117
Did not check the SincCoeffs::get code – just assumed it works – I assume this is creates a kernel which can be matrix multiplied by the image? And the parts of the bounding box which are not in the aperture will be set to zero?
Code to sum elliptical PixelRegion is redundant, and looks like it could be a subroutine. But I wouldn't change it at this point.
CircularApertureFlux.cc – should probably add a comment about measure(), and the fact that this is not a wrappable algorithm.
isn't the specification of a radius > maxSincRadius an error?
baseLib.i – sent you the note about watching out not to squash the GaussianFlux input change to FootprintCentroidShapeInput
ApertureFlux.h – I would have expected the test to be getA() <= ctrl.maxSincRadius. Perhaps I missed something. Doesn't matter at the moment, but could eventually
testApertureFlux.py – look good
plugins.py – not incorrect wrap of ApertureFluxAlgorithm as I mention in an email
Perry Gee
added a comment - ApertureFlux.cc
typo in line 117
Did not check the SincCoeffs::get code – just assumed it works – I assume this is creates a kernel which can be matrix multiplied by the image? And the parts of the bounding box which are not in the aperture will be set to zero?
Code to sum elliptical PixelRegion is redundant, and looks like it could be a subroutine. But I wouldn't change it at this point.
CircularApertureFlux.cc – should probably add a comment about measure(), and the fact that this is not a wrappable algorithm.
isn't the specification of a radius > maxSincRadius an error?
baseLib.i – sent you the note about watching out not to squash the GaussianFlux input change to FootprintCentroidShapeInput
ApertureFlux.h – I would have expected the test to be getA() <= ctrl.maxSincRadius. Perhaps I missed something. Doesn't matter at the moment, but could eventually
testApertureFlux.py – look good
plugins.py – not incorrect wrap of ApertureFluxAlgorithm as I mention in an email Comment contains questions or suggestions.
I also tested this at the 10% level against the old ApertureFlux in meas_algorithms, just to be sure the results you are getting are in the right ball park.
Perry Gee
added a comment - Comment contains questions or suggestions.
I also tested this at the 10% level against the old ApertureFlux in meas_algorithms, just to be sure the results you are getting are in the right ball park. ApertureFlux.cc
typo in line 117
Fixed.
Did not check the SincCoeffs::get code – just assumed it works – I assume this is creates a kernel which can be matrix multiplied by the image? And the parts of the bounding box which are not in the aperture will be set to zero?
Correct, though it's just a dot product, not a matrix product.
Code to sum elliptical PixelRegion is redundant, and looks like it could be a subroutine. But I wouldn't change it at this point.
Yeah...it looks like it should be something that could be factored out, but I couldn't come up with a way to do so that didn't ultimately add more code.
CircularApertureFlux.cc – should probably add a comment about measure(), and the fact that this is not a wrappable algorithm.
Done.
isn't the specification of a radius > maxSincRadius an error?
No - a radius larger than maxSincRadius causes the algorithm to use the naive algorithm for that radius instead.
baseLib.i – sent you the note about watching out not to squash the GaussianFlux input change to FootprintCentroidShapeInput
Fixed.
ApertureFlux.h – I would have expected the test to be getA() <= ctrl.maxSincRadius. Perhaps I missed something. Doesn't matter at the moment, but could eventually
As noted in the docs for the maxSincRadius control field, this is intentional - the idea is that we want to use the sinc algorithm for small radii, where it's more accurate, and use naive flux for larger radii where the difference is relatively unimportant and the sinc code is slow. By basing the threshold on the minor axis, we're erring on the side of accuracy.
plugins.py – not incorrect wrap of ApertureFluxAlgorithm as I mention in an email
Fixed.
Added a starting point on u/jbosch/
DM-837: it's a minimal conversion of the ApertureFlux code from meas_algorithms by Perry (originally on tickets/DM-976) that I've rebased here. Still need to solveDM-836before we make significant progress here, though.