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

New spatially-variant PhotoCalib object

    XMLWordPrintable

    Details

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

      Description

      To improve jointcal's photometric fitting, we need a way to represent a spatially-varying photometric model. Our current Calib object is a constant flux/magnitude zero point (fluxMag0) per ccd/visit. We expect to want to handle a spatially varying component (whether due to sky, optical system, filters, a shift across a CCD, or some combination thereof), and jointcal should be able to fit such a model. As an example, HSC's meas_mosaic fits a 7th order polynomial on the focal plane, plus a fluxMag0 per CCD.

      I propose a replacement for lsst::afw::image::Calib that is built on top of an lsst::afw::math::BoundedField, defined in terms of the conversion from counts to flux in maggies. It will have countsToMaggies() and countsToMagnitudes() methods (replacing the current getMagnitude()) that take counts and/or a point, as well as a SourceRecord or SourceCatalog. The new PhotoCalib will behave the same as the old Calib for spatially invariant scaling (i.e. only fluxMag0 is defined).

      The class docstring for the new PhotoCalib object is as follows:

      /**
       * @brief      The photometric calibration of an exposure.
       *
       * A PhotoCalib is a BoundedField (a function with a specified domain) that converts between calibrated
       * counts-on-chip (ADU) to flux and magnitude. It is defined in terms of "maggies", which are a linear
       * unit defined in SDSS: http://www.sdss.org/dr12/algorithms/magnitudes/#nmgy
       *
       * PhotoCalib is immutable.
       *
       * The spatially varying flux/magnitude zero point is defined such that,
       * at a position (x,y) in the domain of the boundedField zeroPoint
       * and for a given measured source counts:
       *     zeroPoint(x,y) * counts = flux (in maggies)
       * while the errors (constant on the domain) are defined as:
       *     sqrt(countsSigma^2 + zeroPointSigma^2) = fluxSigma (in maggies)
       */
      

      You can see the full API for the new PhotoCalib on this gist.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            LDM-151 (draft version, §9.9 in the copy I'm looking at) defines a new PhotoCalib object which represents "spatially and wavelength-dependent photometric calibration".

            I know that you've discussed your new design for Calib with Jim Bosch. Can one or both of you comment on how the new Calib you're proposing relates to the LDM-151 design? That could be both at the fairly trivial level (do you propose to rename it PhotoCalib to follow the LDM-151 terminology?) and in more substantive terms (how extensible will your your design be to meet the full requirements as described in LDM-151?).

            Show
            swinbank John Swinbank added a comment - LDM-151 ( draft version , §9.9 in the copy I'm looking at) defines a new PhotoCalib object which represents "spatially and wavelength-dependent photometric calibration". I know that you've discussed your new design for Calib with Jim Bosch . Can one or both of you comment on how the new Calib you're proposing relates to the LDM-151 design? That could be both at the fairly trivial level (do you propose to rename it PhotoCalib to follow the LDM-151 terminology?) and in more substantive terms (how extensible will your your design be to meet the full requirements as described in LDM-151?).
            Hide
            jbosch Jim Bosch added a comment -

            If I wasn't concerned with history and backwards compatibility, I do like the name PhotoCalib somewhat better, and I felt it might be a clearer term for readers of LDM-151 who were not familiar with the stack. But I have no problem with continuing to call the code object Calib in the interest of backwards compatibility, or with renaming the term in LDM-151 for consistency with it.

            I view the design John Parejko has proposed as an intermediate step towards what we eventually need: it adds spatial dependence without adding wavelength dependence, and does so in a way that we think will make it easy (and minimally disruptive) to add wavelength dependence in the future. When that happens, I expect us to add versions of most of the conversion functions that also ask for some sort of SED object, while leaving the existing ones to assume some canonical SED. I don't expect us to add a wavelength dimension to BoundedField, though; the methods exposing a BoundedField here will provide a way to get the spatial variation assuming a given or canonical SED, which we'll frequently want in order to scale images.

            John Parejko and Russell Owen: I hadn't appreciated this in our previous discussion, but I think the above suggests we should add composition by making a BoundedField implementation backed by AST or our own composition-of-BoundedField implementation, rather than replacing the BoundedField objects with AST objects directly. If there is some other strong reason to have AST objects replace BoundedField (I can't think of one), we should consider removing it from the public interface here or clearly document those parts of the interface as placeholders.

            Show
            jbosch Jim Bosch added a comment - If I wasn't concerned with history and backwards compatibility, I do like the name PhotoCalib somewhat better, and I felt it might be a clearer term for readers of LDM-151 who were not familiar with the stack. But I have no problem with continuing to call the code object Calib in the interest of backwards compatibility, or with renaming the term in LDM-151 for consistency with it. I view the design John Parejko has proposed as an intermediate step towards what we eventually need: it adds spatial dependence without adding wavelength dependence, and does so in a way that we think will make it easy (and minimally disruptive) to add wavelength dependence in the future. When that happens, I expect us to add versions of most of the conversion functions that also ask for some sort of SED object, while leaving the existing ones to assume some canonical SED. I don't expect us to add a wavelength dimension to BoundedField , though; the methods exposing a BoundedField here will provide a way to get the spatial variation assuming a given or canonical SED, which we'll frequently want in order to scale images. John Parejko and Russell Owen : I hadn't appreciated this in our previous discussion, but I think the above suggests we should add composition by making a BoundedField implementation backed by AST or our own composition-of- BoundedField implementation, rather than replacing the BoundedField objects with AST objects directly. If there is some other strong reason to have AST objects replace BoundedField (I can't think of one), we should consider removing it from the public interface here or clearly document those parts of the interface as placeholders.
            Hide
            Parejkoj John Parejko added a comment -

            I'm more than happy to change the name to PhotoCalib: that makes the intention clearer. Since this is breaking the current API anyway (e.g. getMagnitudes()->countsToMagnitudes()), changing the object's name now seems reasonable.

            As to the rest of LDM-151, this seems like a reasonable step towards the end goal of the object described there, as Jim Bosch says above. Adding wavelength dependence seems like a logical next step (much like this one collapses to the current Calib for spatially-constant zero points). I'd rather work toward that incrementally as-needed, instead of trying to implement it all at once.

            To a few of the points raised in LDM-151 9.9:

            • BoundedField is already persistable, so this object should be trivially persistable.
            • One could add absolute calibration to this by adding countsToJanskys() methods that take into account knowledge of the absolute calibration of the system.
            • Uncertainty is currently constant across the field, but could be extended in the future with something more complicated.
            • I'm not entirely sure what the point about representating rescalings means, but I believe it is included via the computeScalingTo method.

            To Jim Bosch's comments re: AST and BoundedField, the current RFC only says that we should implement PhotoCalib with BoundedField for now. If we want to replace that with AST objects (possibly a good route to adding wavelength dependence), I'm fine with that, or with just backing BoundedField with AST. I don't have an opinion on that at the moment, and I don't think it's directly relevant to this RFC (but it's certainly a good topic to keep in mind for the future).

            Show
            Parejkoj John Parejko added a comment - I'm more than happy to change the name to PhotoCalib: that makes the intention clearer. Since this is breaking the current API anyway (e.g. getMagnitudes() -> countsToMagnitudes() ), changing the object's name now seems reasonable. As to the rest of LDM-151, this seems like a reasonable step towards the end goal of the object described there, as Jim Bosch says above. Adding wavelength dependence seems like a logical next step (much like this one collapses to the current Calib for spatially-constant zero points). I'd rather work toward that incrementally as-needed, instead of trying to implement it all at once. To a few of the points raised in LDM-151 9.9: BoundedField is already persistable, so this object should be trivially persistable. One could add absolute calibration to this by adding countsToJanskys() methods that take into account knowledge of the absolute calibration of the system. Uncertainty is currently constant across the field, but could be extended in the future with something more complicated. I'm not entirely sure what the point about representating rescalings means, but I believe it is included via the computeScalingTo method. To Jim Bosch 's comments re: AST and BoundedField, the current RFC only says that we should implement PhotoCalib with BoundedField for now. If we want to replace that with AST objects (possibly a good route to adding wavelength dependence), I'm fine with that, or with just backing BoundedField with AST. I don't have an opinion on that at the moment, and I don't think it's directly relevant to this RFC (but it's certainly a good topic to keep in mind for the future).
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - - edited

            I'm fine with it. It's a good step in the right direction. My only question are mostly how this fits in to Jim Bosch, Merlin Fisher-Levine, and Robert Lupton 's vision of what will be necessary to report such calibration information.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - - edited I'm fine with it. It's a good step in the right direction. My only question are mostly how this fits in to Jim Bosch , Merlin Fisher-Levine , and Robert Lupton 's vision of what will be necessary to report such calibration information.
            Hide
            rhl Robert Lupton added a comment -

            My apologies for the delay.

            As an object that's ultimately returned from the jointcal photometric fitter this sounds fine – the zero point at a point if we neglect the colour terms is well described by a BoundedBox. We're going to have to face the SED-dependance at some point soonish, but it's clearly out of scope for now.

            Internally to the fitter the object must be much more complicated – it's a combination of things which depend on the detectors (so a tiled set of BoundedBoxes??), the filters (a function of position, maybe just a central wavelength and an equivalent width), the atmosphere (taken as uniform over each visit for now, but varying in time), the extinction (let's say gray for now), the distortion (the flats applied may or may not have taken this out – that depends on whether the sky has been subtracted), and probably other things that I can't recall just now. The fitter has to be able to solve for these simultaneously (with suitable precautions to return something sane for things that are under-constrained or nearly-fixed), and we need to be able to interrogate the results of the fitter for the separate components. For example, if I take the filter to be uniform (or described by low-order radial terms in equivalent width) I want to be able to ask for the CCD efficiency as a function of pixel position from a set of dithered images.

            Similar comments apply to the astrometric solver of course.

            Show
            rhl Robert Lupton added a comment - My apologies for the delay. As an object that's ultimately returned from the jointcal photometric fitter this sounds fine – the zero point at a point if we neglect the colour terms is well described by a BoundedBox . We're going to have to face the SED-dependance at some point soonish, but it's clearly out of scope for now. Internally to the fitter the object must be much more complicated – it's a combination of things which depend on the detectors (so a tiled set of BoundedBoxes ??), the filters (a function of position, maybe just a central wavelength and an equivalent width), the atmosphere (taken as uniform over each visit for now, but varying in time), the extinction (let's say gray for now), the distortion (the flats applied may or may not have taken this out – that depends on whether the sky has been subtracted), and probably other things that I can't recall just now. The fitter has to be able to solve for these simultaneously (with suitable precautions to return something sane for things that are under-constrained or nearly-fixed), and we need to be able to interrogate the results of the fitter for the separate components. For example, if I take the filter to be uniform (or described by low-order radial terms in equivalent width) I want to be able to ask for the CCD efficiency as a function of pixel position from a set of dithered images. Similar comments apply to the astrometric solver of course.
            Hide
            Parejkoj John Parejko added a comment -

            Robert Lupton are you proposing that we instead implement that more complicated model now in PhotoCalib? The goal of this proposal is to build up a PhotoCalib (and, by extension, the jointcal model) one step at a time, as needed.

            Show
            Parejkoj John Parejko added a comment - Robert Lupton are you proposing that we instead implement that more complicated model now in PhotoCalib? The goal of this proposal is to build up a PhotoCalib (and, by extension, the jointcal model) one step at a time, as needed.
            Hide
            rhl Robert Lupton added a comment - - edited

            I think that we (you!) should continue to get jointcal photometry working. My second paragraph was intended to make that clear – this is fine as an output of jointcal for now, and maybe in the long-run.

            What I am concerned about in this context is that your proposed PhotoCalib is not the sole long-term output of jointcal for the reasons given, so I want to be sure that this RFC is not taken to define what PhotoCalib will become.

            Show
            rhl Robert Lupton added a comment - - edited I think that we (you!) should continue to get jointcal photometry working. My second paragraph was intended to make that clear – this is fine as an output of jointcal for now, and maybe in the long-run. What I am concerned about in this context is that your proposed PhotoCalib is not the sole long-term output of jointcal for the reasons given, so I want to be sure that this RFC is not taken to define what PhotoCalib will become.
            Hide
            rowen Russell Owen added a comment - - edited

            I would hope that the BoundedField itself could combine the different models required to represent the spatially varying component of the PhotoCalib. That seems natural if BoundedField is defined in terms of AST models. Such a model could be expanded to include wavelength dependence, as well.

            Show
            rowen Russell Owen added a comment - - edited I would hope that the BoundedField itself could combine the different models required to represent the spatially varying component of the PhotoCalib. That seems natural if BoundedField is defined in terms of AST models. Such a model could be expanded to include wavelength dependence, as well.
            Hide
            Parejkoj John Parejko added a comment -

            I'm proposing this now, because it's the minimum thing I need now, and we don't have a way to represent such a calib.

            As I said above re:LDM-151, this is the proposal for a new PhotoCalib that has room to be extended in the future. We already know there needs to be an SED term eventually, and as Russell Owen says, one could make a composite BoundedField to represent a more complicated set of transforms.

            Show
            Parejkoj John Parejko added a comment - I'm proposing this now, because it's the minimum thing I need now, and we don't have a way to represent such a calib. As I said above re:LDM-151, this is the proposal for a new PhotoCalib that has room to be extended in the future. We already know there needs to be an SED term eventually, and as Russell Owen says, one could make a composite BoundedField to represent a more complicated set of transforms.
            Hide
            jbosch Jim Bosch added a comment -

            My vision for the composition and tiling was indeed that it happen within the BoundedField.

            I believe AST is an option for implementing that, though I should emphasize that my previous comment about where AST would go should not be taken to imply that I endorse using it here. Those kinds of implementation questions do to be considered in the larger context Robert Lupton brought up (and probably not on this ticket) - for example, for fitting we probably need to have objects that are not only composable/tileable but differentiable w.r.t. some set of parameters as well, and hence we may find ourselves having to reimplement a lot of what we may have thought AST would provide for us.

            Show
            jbosch Jim Bosch added a comment - My vision for the composition and tiling was indeed that it happen within the BoundedField . I believe AST is an option for implementing that, though I should emphasize that my previous comment about where AST would go should not be taken to imply that I endorse using it here. Those kinds of implementation questions do to be considered in the larger context Robert Lupton brought up (and probably not on this ticket) - for example, for fitting we probably need to have objects that are not only composable/tileable but differentiable w.r.t. some set of parameters as well, and hence we may find ourselves having to reimplement a lot of what we may have thought AST would provide for us.
            Hide
            krughoff Simon Krughoff added a comment -

            This proposal sounds good to me. It seems like the current proposal can go through with the understanding that the extensions need in the future may be available through a re-design of BoundedField.

            Show
            krughoff Simon Krughoff added a comment - This proposal sounds good to me. It seems like the current proposal can go through with the understanding that the extensions need in the future may be available through a re-design of BoundedField .
            Hide
            Parejkoj John Parejko added a comment -

            Adopted as written above. Implementation ticket is DM-9192.

            Show
            Parejkoj John Parejko added a comment - Adopted as written above. Implementation ticket is DM-9192 .
            Hide
            Parejkoj John Parejko added a comment -

            NOTE: One of the various changes in the final implementation is that I've removed the throwOnNegativeCounts argument, as it appears to be almost never used in the current stack.

            Show
            Parejkoj John Parejko added a comment - NOTE: One of the various changes in the final implementation is that I've removed the throwOnNegativeCounts argument, as it appears to be almost never used in the current stack.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Watchers:
              Dominique Boutigny, Jim Bosch, John Parejko, John Swinbank, Michael Wood-Vasey [X] (Inactive), Pierre Astier, Robert Lupton, Russell Owen, Simon Krughoff, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  CI Builds

                  No builds found.