Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-796

Default Psf position argument considered harmful

    XMLWordPrintable

Details

    • RFC
    • Status: Adopted
    • Resolution: Unresolved
    • DM
    • None

    Description

      Most of the Psf methods that can compute something at a particular point in an image allow for the actual position argument to be omitted, in which case an "average" position is used by default. See many of the methods in Psf.h  This pattern can easily lead to problems when either:

      In cases where the position really is intended to be the average position, it's easy enough to query the Psf object that the programmer already has access to, e.g.:

      psf.computeImage(psf.getAveragePosition())

      Since this pattern appears to be dangerous and unnecessary, I propose to deprecate the no-position-argument interface in each of:

      Psf::computeImage
      Psf::computeKernelImage
      Psf::computePeak
      Psf::computeApertureFlux
      Psf::computeShape
      Psf::getLocalKernel
      Psf::computeBBox
      Psf::computeImageBBox
      Psf::computeKernelBBox

      and their python equivalents.

      (I haven't tried, but I'm guessing we could implement this in c++ by swapping the default parametered methods for overloads).

      Second, Psf::getAveragePosition() has a virtual default implementation, which just returns Point2D(). It is up to derived classes to override this if required, which should be the case for all non-constant Psf models.  However, I'm pretty sure I didn't do this right in meas_extensions_piff.  So part (b) of this RFC is to add a Psf::_averagePosition member variable and require constructors to initialize it. This is effectively already done in a number of derived classes, e.g.,

      Making it mandatory will prevent any future Psf derived classes from naively inheriting a default implementation.

      Attachments

        Issue Links

          Activity

            krzys Krzysztof Findeisen added a comment - - edited

            for the deprecation. Splitting the default into a pair of overloads will work, as you described, though you will also need to make changes at the pybind11 level to get a smart deprecation warning in Python.

            The proposed _averagePosition seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of getAveragePosition, even if that leads to some code duplication.

            krzys Krzysztof Findeisen added a comment - - edited for the deprecation. Splitting the default into a pair of overloads will work, as you described, though you will also need to make changes at the pybind11 level to get a smart deprecation warning in Python. The proposed _averagePosition seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of getAveragePosition , even if that leads to some code duplication.

            The proposed _averagePosition seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of getAveragePosition, even if that leads to some code duplication.

            My thought was to add a constructor argument to Psf::Psf.  But just removing the default implementation of Psf::getAveragePosition() probably is simpler.  I'm happy to go with that.

            jmeyers314 Joshua Meyers added a comment - The proposed  _averagePosition  seems a bit error-prone – how will you enforce the initialization requirement? It might be better to remove the default definition of  getAveragePosition , even if that leads to some code duplication. My thought was to add a constructor argument to Psf::Psf.  But just removing the default implementation of Psf::getAveragePosition() probably is simpler.  I'm happy to go with that.

            Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then?

            krzys Krzysztof Findeisen added a comment - Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then?

            Good point, a mandatory constructor argument would work as well. Would it still be necessary for getAveragePosition to be overrideable then?

            As far as I can tell, in this case it'd be sufficient for getAveragePosition to just return _averagePosition (and not be overridable). 

            The trickiest bit might be adjusting the code in CoaddPsf::computeAveragePosition such that it can be used in an initializer list for _averagePosition.  Or we just initialize in the ctor body I guess, and leave _averagePosition non-const, though I like the idea of making it const if possible.

            jmeyers314 Joshua Meyers added a comment - Good point, a mandatory constructor argument would work as well. Would it still be necessary for  getAveragePosition  to be overrideable then? As far as I can tell, in this case it'd be sufficient for getAveragePosition to just return _averagePosition (and not be overridable).  The trickiest bit might be adjusting the code in CoaddPsf::computeAveragePosition such that it can be used in an initializer list for _averagePosition .  Or we just initialize in the ctor body I guess, and leave _averagePosition non-const, though I like the idea of making it const if possible.
            erykoff Eli Rykoff added a comment -

            I 1000% endorse this RFC for deprecating the no-position-argument interface for these methods.

            I have no opinion on the exact implementation of the average position computation.

            erykoff Eli Rykoff added a comment - I 1000% endorse this RFC for deprecating the no-position-argument interface for these methods. I have no opinion on the exact implementation of the average position computation.
            tjenness Tim Jenness added a comment -

            Escalating since this is an API change.

            tjenness Tim Jenness added a comment - Escalating since this is an API change.

            Adopting without committing to a particular implementation for getAveragePosition().  The important bit is that the default implementation is removed so that derived classes don't accidentally inherit it.

            jmeyers314 Joshua Meyers added a comment - Adopting without committing to a particular implementation for getAveragePosition() .  The important bit is that the default implementation is removed so that derived classes don't accidentally inherit it.

            I was recently bitten by this when developing GAaP, partly because I didn't realize that the average position can be outside the bounding box if I get a small postage stamp from a coadd. Full support from me to mandate a position argument.

            kannawad Arun Kannawadi added a comment - I was recently bitten by this when developing GAaP, partly because I didn't realize that the average position can be outside the bounding box if I get a small postage stamp from a coadd. Full support from me to mandate a position argument.

            People

              jmeyers314 Joshua Meyers
              jmeyers314 Joshua Meyers
              Arun Kannawadi, Eli Rykoff, Joshua Meyers, Krzysztof Findeisen, Tim Jenness
              Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Planned End:

                Jenkins

                  No builds found.