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

Backwards compatibility API for Calib->PhotoCalib transition

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      The API of the new PhotoCalib object is entirely different from the old Calib API: it is defined in terms of a flux calibration factor instead of a magnitude zero point, methods are named e.g. instFluxToMagnitude instead of getMagnitude, and it never raises an exception when given a negative instrumental flux. To ease the transition to PhotoCalib, I propose that PhotoCalib gain the following deprecated methods from Calib until v18.0:

          /**
           * Return an instrumental flux and error (in ADU) given magnitude and magnitude error.
           *
           * Assumes that the errors are small and uncorrelated.
           *
           * @param mag The AB magnitude of the object.
           * @param magErr The error in the magnitude.
           */
          std::pair<double, double> getFlux(double const mag, double const magErr) const;
          ndarray::Array<double, 1> getFlux(ndarray::Array<double const, 1> const& mag) const;
          std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getFlux(
                  ndarray::Array<double const, 1> const& mag, ndarray::Array<double const, 1> const& magErr) const;
       
          /**
           * Return a magnitude and magnitude error given instrumental flux and error (in ADU).
           *
           * @param instFlux The measured instFlux of the object (ADU).
           * @param instFluxErr The error in the measured instFlux (ADU).
           */
          std::pair<double, double> getMagnitude(double const instFlux, double const instFluxErr) const;
          ndarray::Array<double, 1> getMagnitude(ndarray::Array<double const, 1> const& instFlux) const;
          std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getMagnitude(
                  ndarray::Array<double const, 1> const& instFlux,
                  ndarray::Array<double const, 1> const& instFluxErr) const;
      

      Other methods on Calib that we will not provide backwards-compatible deprecated methods for:

      • setFluxMag0: Invalid; PhotoCalib is immutable.
      • getFluxMag0: PhotoCalib has getInstFluxAtZeroMagnitude, which only returns the 0-point and not the 0-point error, which has a non-trivial relationship with the calibration error. This was most commonly used to construct a new Calib, about which see the next point.
      • Construction from a "instFlux at zero magnitude" double: this cannot be distinguished from the existing calibrationMean constructor. If such functionality is strongly desired, we could add a makePhotoCalibFromZeroPoint (or similar) free function, but I doubt it is necessary: users rarely have to construct their own photoCalib, and PhotoCalib will be able to read older Exposure files that contain persisted Calibs.
      • get/setThrowOnNegativeFlux: Invalid; PhotoCalib is immutable and never throws in this manner.
      • operator*= and operator/=: Invalid; PhotoCalib is immutable. We could provide operator* and operator/ though (returning a new PhotoCalib), if this functionality was important.

        Attachments

          Issue Links

            Activity

            No builds found.
            Parejkoj John Parejko created issue -
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Link This issue relates to DM-10153 [ DM-10153 ]
            Parejkoj John Parejko made changes -
            Description The API of the new {{PhotoCalib}} object is entirely different from the old {{Calib}} API: it is defined in terms of a flux calibration factor instead of a magnitude zero point, methods are named e.g. {{instFluxToMagnitude}} instead of {{getMagnitude}}, and it never raises an exception when given a negative instrumental flux. To ease the transition to {{PhotoCalib}}, I propose that {{PhotoCalib}} gain the following deprecated methods from {{Calib}} until v18.0:
            {code} /**
                 * Return an instrumental flux and error (in ADU) given magnitude and magnitude error.
                 *
                 * Assumes that the errors are small and uncorrelated.
                 *
                 * @param mag The AB magnitude of the object.
                 * @param magErr The error in the magnitude.
                 */
                std::pair<double, double> getFlux(double const mag, double const magErr) const;
                ndarray::Array<double, 1> getFlux(ndarray::Array<double const, 1> const& mag) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getFlux(
                        ndarray::Array<double const, 1> const& mag, ndarray::Array<double const, 1> const& magErr) const;

                /**
                 * Return a magnitude and magnitude error given instrumental flux and error (in ADU).
                 *
                 * @param instFlux The measured instFlux of the object (ADU).
                 * @param instFluxErr The error in the measured instFlux (ADU).
                 */
                std::pair<double, double> getMagnitude(double const instFlux, double const instFluxErr) const;
                ndarray::Array<double, 1> getMagnitude(ndarray::Array<double const, 1> const& instFlux) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getMagnitude(
                        ndarray::Array<double const, 1> const& instFlux,
                        ndarray::Array<double const, 1> const& instFluxErr) const;
            {code}
            Other methods on {{Calib}} that we will not provide backwards-compatible deprecated methods for:
             * {{setFluxMag0}}: Invalid; PhotoCalib is immutable.
             * {{getFluxMag0}}: PhotoCalib has getInstFluxAtZeroMagnitude, which only returns the 0-point and not the 0-point error, which has a non-trivial relationship with the calibration error. This was most commonly used to construct a new {{Calib}}, about which see the next point.
             * Construction from a "instFlux at zero magnitude" double: this cannot be distinguished from the existing {{calibrationMean}} constructor. If such functionality is strongly desired, we could add a {{makePhotoCalibFromZeroPoint}} (or similar) free function, but I doubt it is necessary: users rarely have to construct their own photoCalib, and PhotoCalib will be able to read older {{Exposure}} files that contain persisted {{Calib}}s.
             * {{get/setThrowOnNegativeFlux}}: Invalid; PhotoCalib is immutable and never throws in this manner.
            The API of the new {{PhotoCalib}} object is entirely different from the old {{Calib}} API: it is defined in terms of a flux calibration factor instead of a magnitude zero point, methods are named e.g. {{instFluxToMagnitude}} instead of {{getMagnitude}}, and it never raises an exception when given a negative instrumental flux. To ease the transition to {{PhotoCalib}}, I propose that {{PhotoCalib}} gain the following deprecated methods from {{Calib}} until v18.0:
            {code} /**
                 * Return an instrumental flux and error (in ADU) given magnitude and magnitude error.
                 *
                 * Assumes that the errors are small and uncorrelated.
                 *
                 * @param mag The AB magnitude of the object.
                 * @param magErr The error in the magnitude.
                 */
                std::pair<double, double> getFlux(double const mag, double const magErr) const;
                ndarray::Array<double, 1> getFlux(ndarray::Array<double const, 1> const& mag) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getFlux(
                        ndarray::Array<double const, 1> const& mag, ndarray::Array<double const, 1> const& magErr) const;

                /**
                 * Return a magnitude and magnitude error given instrumental flux and error (in ADU).
                 *
                 * @param instFlux The measured instFlux of the object (ADU).
                 * @param instFluxErr The error in the measured instFlux (ADU).
                 */
                std::pair<double, double> getMagnitude(double const instFlux, double const instFluxErr) const;
                ndarray::Array<double, 1> getMagnitude(ndarray::Array<double const, 1> const& instFlux) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getMagnitude(
                        ndarray::Array<double const, 1> const& instFlux,
                        ndarray::Array<double const, 1> const& instFluxErr) const;
            {code}
            Other methods on {{Calib}} that we will not provide backwards-compatible deprecated methods for:
             * {{setFluxMag0}}: Invalid; PhotoCalib is immutable.
             * {{getFluxMag0}}: PhotoCalib has getInstFluxAtZeroMagnitude, which only returns the 0-point and not the 0-point error, which has a non-trivial relationship with the calibration error. This was most commonly used to construct a new {{Calib}}, about which see the next point.
             * Construction from a "instFlux at zero magnitude" double: this cannot be distinguished from the existing {{calibrationMean}} constructor. If such functionality is strongly desired, we could add a {{makePhotoCalibFromZeroPoint}} (or similar) free function, but I doubt it is necessary: users rarely have to construct their own photoCalib, and PhotoCalib will be able to read older {{Exposure}} files that contain persisted \{\{Calibs}}.
             * {{get/setThrowOnNegativeFlux}}: Invalid; PhotoCalib is immutable and never throws in this manner.
            Parejkoj John Parejko made changes -
            Description The API of the new {{PhotoCalib}} object is entirely different from the old {{Calib}} API: it is defined in terms of a flux calibration factor instead of a magnitude zero point, methods are named e.g. {{instFluxToMagnitude}} instead of {{getMagnitude}}, and it never raises an exception when given a negative instrumental flux. To ease the transition to {{PhotoCalib}}, I propose that {{PhotoCalib}} gain the following deprecated methods from {{Calib}} until v18.0:
            {code} /**
                 * Return an instrumental flux and error (in ADU) given magnitude and magnitude error.
                 *
                 * Assumes that the errors are small and uncorrelated.
                 *
                 * @param mag The AB magnitude of the object.
                 * @param magErr The error in the magnitude.
                 */
                std::pair<double, double> getFlux(double const mag, double const magErr) const;
                ndarray::Array<double, 1> getFlux(ndarray::Array<double const, 1> const& mag) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getFlux(
                        ndarray::Array<double const, 1> const& mag, ndarray::Array<double const, 1> const& magErr) const;

                /**
                 * Return a magnitude and magnitude error given instrumental flux and error (in ADU).
                 *
                 * @param instFlux The measured instFlux of the object (ADU).
                 * @param instFluxErr The error in the measured instFlux (ADU).
                 */
                std::pair<double, double> getMagnitude(double const instFlux, double const instFluxErr) const;
                ndarray::Array<double, 1> getMagnitude(ndarray::Array<double const, 1> const& instFlux) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getMagnitude(
                        ndarray::Array<double const, 1> const& instFlux,
                        ndarray::Array<double const, 1> const& instFluxErr) const;
            {code}
            Other methods on {{Calib}} that we will not provide backwards-compatible deprecated methods for:
             * {{setFluxMag0}}: Invalid; PhotoCalib is immutable.
             * {{getFluxMag0}}: PhotoCalib has getInstFluxAtZeroMagnitude, which only returns the 0-point and not the 0-point error, which has a non-trivial relationship with the calibration error. This was most commonly used to construct a new {{Calib}}, about which see the next point.
             * Construction from a "instFlux at zero magnitude" double: this cannot be distinguished from the existing {{calibrationMean}} constructor. If such functionality is strongly desired, we could add a {{makePhotoCalibFromZeroPoint}} (or similar) free function, but I doubt it is necessary: users rarely have to construct their own photoCalib, and PhotoCalib will be able to read older {{Exposure}} files that contain persisted \{\{Calibs}}.
             * {{get/setThrowOnNegativeFlux}}: Invalid; PhotoCalib is immutable and never throws in this manner.
            The API of the new {{PhotoCalib}} object is entirely different from the old {{Calib}} API: it is defined in terms of a flux calibration factor instead of a magnitude zero point, methods are named e.g. {{instFluxToMagnitude}} instead of {{getMagnitude}}, and it never raises an exception when given a negative instrumental flux. To ease the transition to {{PhotoCalib}}, I propose that {{PhotoCalib}} gain the following deprecated methods from {{Calib}} until v18.0:
            {code} /**
                 * Return an instrumental flux and error (in ADU) given magnitude and magnitude error.
                 *
                 * Assumes that the errors are small and uncorrelated.
                 *
                 * @param mag The AB magnitude of the object.
                 * @param magErr The error in the magnitude.
                 */
                std::pair<double, double> getFlux(double const mag, double const magErr) const;
                ndarray::Array<double, 1> getFlux(ndarray::Array<double const, 1> const& mag) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getFlux(
                        ndarray::Array<double const, 1> const& mag, ndarray::Array<double const, 1> const& magErr) const;

                /**
                 * Return a magnitude and magnitude error given instrumental flux and error (in ADU).
                 *
                 * @param instFlux The measured instFlux of the object (ADU).
                 * @param instFluxErr The error in the measured instFlux (ADU).
                 */
                std::pair<double, double> getMagnitude(double const instFlux, double const instFluxErr) const;
                ndarray::Array<double, 1> getMagnitude(ndarray::Array<double const, 1> const& instFlux) const;
                std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> getMagnitude(
                        ndarray::Array<double const, 1> const& instFlux,
                        ndarray::Array<double const, 1> const& instFluxErr) const;
            {code}
            Other methods on {{Calib}} that we will not provide backwards-compatible deprecated methods for:
             * {{setFluxMag0}}: Invalid; PhotoCalib is immutable.
             * {{getFluxMag0}}: PhotoCalib has getInstFluxAtZeroMagnitude, which only returns the 0-point and not the 0-point error, which has a non-trivial relationship with the calibration error. This was most commonly used to construct a new {{Calib}}, about which see the next point.
             * Construction from a "instFlux at zero magnitude" double: this cannot be distinguished from the existing {{calibrationMean}} constructor. If such functionality is strongly desired, we could add a {{makePhotoCalibFromZeroPoint}} (or similar) free function, but I doubt it is necessary: users rarely have to construct their own photoCalib, and PhotoCalib will be able to read older {{Exposure}} files that contain persisted {{Calibs}}.
             * {{get/setThrowOnNegativeFlux}}: Invalid; PhotoCalib is immutable and never throws in this manner.
             * {{operator*=}} and {{operator/=}}: Invalid; PhotoCalib is immutable. We could provide {{operator*}} and {{operator/}} though (returning a new PhotoCalib), if this functionality was important.
            Parejkoj John Parejko made changes -
            Watchers Jim Bosch, John Parejko, John Swinbank, Paul Price, Robert Lupton, Tim Jenness, Yusra AlSayyad [ Jim Bosch, John Parejko, John Swinbank, Paul Price, Robert Lupton, Tim Jenness, Yusra AlSayyad ] Jim Bosch, John Parejko, John Swinbank, Leanne Guy, Paul Price, Robert Lupton, Tim Jenness, Yusra AlSayyad [ Jim Bosch, John Parejko, John Swinbank, Leanne Guy, Paul Price, Robert Lupton, Tim Jenness, Yusra AlSayyad ]
            Parejkoj John Parejko made changes -
            Link This issue relates to RFC-289 [ RFC-289 ]
            Parejkoj John Parejko made changes -
            Link This issue relates to RFC-575 [ RFC-575 ]
            Hide
            swinbank John Swinbank added a comment - - edited

            I'd like the DM-CCB to approve this suggestion. However, I suggest that the DM-CCB should invite expert opinion from Robert Lupton and/or Jim Bosch before making a ruling.

            Show
            swinbank John Swinbank added a comment - - edited I'd like the DM-CCB to approve this suggestion. However, I suggest that the DM-CCB should invite expert opinion from Robert Lupton and/or Jim Bosch before making a ruling.
            swinbank John Swinbank made changes -
            Status Proposed [ 10805 ] Flagged [ 10606 ]
            Hide
            jbosch Jim Bosch added a comment - - edited

            I think this is broadly fine.  One possible adjustment I think I'd recommend (apologies for not coming up with this when I was asked to comment on an earlier version of this proposal): we could stil retain (and of course deprecate) get/setThrowOnNegativeFlux as no-ops; I doubt these are used often (if at all) in pipeline code, but they could be quite common in notebooks and other analysis code.

            Show
            jbosch Jim Bosch added a comment - - edited I think this is broadly fine.  One possible adjustment I think I'd recommend (apologies for not coming up with this when I was asked to comment on an earlier version of this proposal): we could stil retain (and of course deprecate) get/setThrowOnNegativeFlux as no-ops; I doubt these are used often (if at all) in pipeline code, but they could be quite common in notebooks and other analysis code.
            Parejkoj John Parejko made changes -
            Planned End 20/Feb/19 10:49 PM 22/Feb/19 10:49 PM
            gcomoretto Gabriele Comoretto [X] (Inactive) made changes -
            Remote Link This issue links to "Page (Confluence)" [ 19818 ]
            Hide
            rhl Robert Lupton added a comment -

            I think that this is fine (with Jim's proposed addition). I don't understand why getFluxMag0 cannot return a the error as well as the zero point, but if it is only used to create new {{Calib}}s (which seems likely) maybe it's not worth reporting.

            I am not yet clear about how/if we'll actually remove deprecated methods. I assume that a user will be able to disable the deprecation messages.

            Show
            rhl Robert Lupton added a comment - I think that this is fine (with Jim's proposed addition). I don't understand why getFluxMag0 cannot return a the error as well as the zero point, but if it is only used to create new {{Calib}}s (which seems likely) maybe it's not worth reporting. I am not yet clear about how/if we'll actually remove deprecated methods. I assume that a user will be able to disable the deprecation messages.
            womullan Wil O'Mullane made changes -
            Status Flagged [ 10606 ] Board Recommended [ 11405 ]
            Hide
            Parejkoj John Parejko added a comment -

            I am not yet clear about how/if we'll actually remove deprecated methods. I assume that a user will be able to disable the deprecation messages.

            I would propose that we remove of all the Calib-related APIs (including the above additions to PhotoCalib, and the getCalib() methods in Exposure and ExposureInfo) either just before releasing v18, or immediately after that.

            Show
            Parejkoj John Parejko added a comment - I am not yet clear about how/if we'll actually remove deprecated methods. I assume that a user will be able to disable the deprecation messages. I would propose that we remove of all the Calib -related APIs (including the above additions to PhotoCalib , and the getCalib() methods in Exposure and ExposureInfo ) either just before releasing v18, or immediately after that.
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-16925 [ DM-16925 ]
            swinbank John Swinbank made changes -
            Status Board Recommended [ 11405 ] Adopted [ 10806 ]
            gcomoretto Gabriele Comoretto [X] (Inactive) made changes -
            Remote Link This issue links to "Page (Confluence)" [ 19938 ]
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Adopted [ 10806 ] Implemented [ 11105 ]
            gcomoretto Gabriele Comoretto [X] (Inactive) made changes -
            Remote Link This issue links to "Page (Confluence)" [ 20324 ]

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Watchers:
              Jim Bosch, John Parejko, John Swinbank, Leanne Guy, Paul Price, Robert Lupton, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  CI Builds

                  No builds found.