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

Rewrite multiple-aperture photometry class

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:

      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

            Hide
            jbosch Jim Bosch added a comment -

            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 solve DM-836 before we make significant progress here, though.

            Show
            jbosch Jim Bosch added a comment - 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 solve DM-836 before we make significant progress here, though.
            Hide
            jbosch Jim Bosch added a comment -

            Assigning to Frossie for review, but not really.

            Show
            jbosch Jim Bosch added a comment - Assigning to Frossie for review, but not really.
            Hide
            jbosch Jim Bosch added a comment -

            Perry, this now ready for you to review.

            Show
            jbosch Jim Bosch added a comment - Perry, this now ready for you to review.
            Hide
            pgee 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

            Show
            pgee 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
            Hide
            pgee 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.

            Show
            pgee 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.
            Hide
            jbosch Jim Bosch added a comment -

            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.

            Show
            jbosch Jim Bosch added a comment - 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.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Perry Gee
              Watchers:
              Frossie Economou, Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.