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

Add getDimensions and/or getBBox method to all PSFs

    Details

    • Templates:
    • Story Points:
      4
    • Sprint:
      DRP S17-1
    • Team:
      Data Release Production

      Description

      In order to be able to psf-match a WarpedPSF or a CoaddPsf, ModelPsfMatch needs to know the dimensions of the WarpedPsf. Currently the only way to get this is to compute the kernel image first. There are cheaper ways to compute the dimensions, starting by asking the undistorted PSFs (either PsfexPsf or PcaPsf) for its dimensions. Simplest solution is to give all PSFs a getDimensions method.

        Issue Links

          Activity

          Hide
          yusra Yusra AlSayyad added a comment -

          This needs a review. It touches 4 packages and I created 4 pull requests. The links on the right are missing the meas_extenstions_psfex pull request:
          https://github.com/lsst/meas_extensions_psfex/pull/16

          I ran ci_hsc to confirm there were no changes in behavior to CoaddPsf.

          Show
          yusra Yusra AlSayyad added a comment - This needs a review. It touches 4 packages and I created 4 pull requests. The links on the right are missing the meas_extenstions_psfex pull request: https://github.com/lsst/meas_extensions_psfex/pull/16 I ran ci_hsc to confirm there were no changes in behavior to CoaddPsf.
          Hide
          jmeyers314 Joshua Meyers added a comment -

          Hi Yusra AlSayyad, this looks generally good to me. I'm curious why you decided to implement computeBBox as opposed to just computeExtent? Is the assumption that the former isn't significantly more compute intensive than the latter, so might as well get both an extent and a center?

          Other than that, most of my comments on github are either very simple things, or just questions I have about the PSF framework.

          Show
          jmeyers314 Joshua Meyers added a comment - Hi Yusra AlSayyad , this looks generally good to me. I'm curious why you decided to implement computeBBox as opposed to just computeExtent? Is the assumption that the former isn't significantly more compute intensive than the latter, so might as well get both an extent and a center? Other than that, most of my comments on github are either very simple things, or just questions I have about the PSF framework.
          Hide
          yusra Yusra AlSayyad added a comment -

          As you can tell by the title of this ticket, I was originally envisioning a computeDimensions method, but settled on computeBBox. Thanks for asking, because the reasoning should be recorded here for future reference.
          1)

          • CoaddPsf and WarpedPsf (in computeBBoxFromTransform) need the full BBox of their component Psfs to compute the bounding BBox.
          • Some component Psfs (PcaPsf for example), contain kernels with getBBox methods.
          • Instead of having CoaddPsf and WarpedPsf read the dimensions and construct the bbox, I assume their contained Psfs best know how to compute their own bbox, and let them do it.

          2) Allows some code reuse within each PSF class: doComputeKernelImage creates an empty image with bbox = computeBbox() (with the exception of PsfexPsf)

          3) A BBox2I doesn't weigh much more than an Extent2I

          4) BBox->dimensions is simpler than dimensions->BBox. As a user: I can confidently write bbox.getDimensions(), whereas I would want to double check that I got the right starting point when faced with odd and even dimensions when I write: BBox2I(Point2I(-dim.getWidth//2, -dim.getHeight//2), dim))

          Show
          yusra Yusra AlSayyad added a comment - As you can tell by the title of this ticket, I was originally envisioning a computeDimensions method, but settled on computeBBox. Thanks for asking, because the reasoning should be recorded here for future reference. 1) CoaddPsf and WarpedPsf (in computeBBoxFromTransform) need the full BBox of their component Psfs to compute the bounding BBox. Some component Psfs (PcaPsf for example), contain kernels with getBBox methods. Instead of having CoaddPsf and WarpedPsf read the dimensions and construct the bbox, I assume their contained Psfs best know how to compute their own bbox, and let them do it. 2) Allows some code reuse within each PSF class: doComputeKernelImage creates an empty image with bbox = computeBbox() (with the exception of PsfexPsf) 3) A BBox2I doesn't weigh much more than an Extent2I 4) BBox->dimensions is simpler than dimensions->BBox. As a user: I can confidently write bbox.getDimensions(), whereas I would want to double check that I got the right starting point when faced with odd and even dimensions when I write: BBox2I(Point2I(-dim.getWidth//2, -dim.getHeight//2), dim))
          Hide
          yusra Yusra AlSayyad added a comment -

          Lets talk https://github.com/lsst/meas_algorithms/pull/54/commits/a5d6c30d171eab2c22d020e8ec015890fbc13608

          In WarpedPsf, the originalPsf image gets padded by the ~half the warping kernel before being warped. The reason we have never encountered this before is because afw.math.warpImage properly pads the edges with 0, if the output bbox is larger than the inside of kernel X image.

          Here are some examples of what the Warped Psf Image looks like for a WarpedPsf adapted from the original unit test.

          The warping kernel is 6 X 6: Point2I(-2,-2), Extent2I(6,6), and stretch is ds9's histogram min/max.
          1) The original code pads the image by 4 in the x direction and 2 in the y direction.
          2) The new code in this pull request pads by 4 in both directions.
          3) Just for illustration this is the result from padding 2 in both directions
          4) Just for illustration no padding.

          The difference between 1 and 2 is the top and bottom rows which are 0 (integer) in the unevenly padded case.

          For a unit test, we could check that the sum of the outer most rows and columns are not exactly 0.

          Show
          yusra Yusra AlSayyad added a comment - Lets talk https://github.com/lsst/meas_algorithms/pull/54/commits/a5d6c30d171eab2c22d020e8ec015890fbc13608 In WarpedPsf, the originalPsf image gets padded by the ~half the warping kernel before being warped. The reason we have never encountered this before is because afw.math.warpImage properly pads the edges with 0, if the output bbox is larger than the inside of kernel X image. Here are some examples of what the Warped Psf Image looks like for a WarpedPsf adapted from the original unit test. The warping kernel is 6 X 6: Point2I(-2,-2), Extent2I(6,6), and stretch is ds9's histogram min/max. 1) The original code pads the image by 4 in the x direction and 2 in the y direction. 2) The new code in this pull request pads by 4 in both directions. 3) Just for illustration this is the result from padding 2 in both directions 4) Just for illustration no padding. The difference between 1 and 2 is the top and bottom rows which are 0 (integer) in the unevenly padded case. For a unit test, we could check that the sum of the outer most rows and columns are not exactly 0.
          Hide
          yusra Yusra AlSayyad added a comment -

          Thanks for the review Joshua Meyers. I made the changes you suggested, and added a unit test to check the padding along with that commit to change the yPad from min to max. When you get a chance, would you take a look at the unit test since it's new code.

          Show
          yusra Yusra AlSayyad added a comment - Thanks for the review Joshua Meyers . I made the changes you suggested, and added a unit test to check the padding along with that commit to change the yPad from min to max. When you get a chance, would you take a look at the unit test since it's new code.
          Hide
          jmeyers314 Joshua Meyers added a comment -

          Thanks Yusra AlSayyad, this looks ready to merge to me.

          Show
          jmeyers314 Joshua Meyers added a comment - Thanks Yusra AlSayyad , this looks ready to merge to me.

            People

            • Assignee:
              yusra Yusra AlSayyad
              Reporter:
              yusra Yusra AlSayyad
              Reviewers:
              Joshua Meyers
              Watchers:
              Jim Bosch, John Swinbank, Joshua Meyers, Yusra AlSayyad
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile