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

Remove exposureId from VisitInfo and add visitId

    XMLWordPrintable

Details

    • RFC
    • Status: Implemented
    • Resolution: Done
    • DM
    • None

    Description

      The exposureId value in visitInfo is not a visit-exposure level identifier, but rather a ccd-exposure level identifier (see docstring for obs.base.CameraMapper._computeCcdExposureId() for the only place where this seems to be specified). Thus, it should not be included in visitInfo, which is only things that are common across the visit.

      We do need a visit-exposure level identifier to be included in visitInfo, so that we can link exposures within visits without using the butler (e.g. without dataRef.dataId['visit']). This RFC likely requires that makeRawVisitInfo() adds the butler visitId to the new visitInfo.visitId, unless there is some other way to access the visitId value besides the above butler call.

      Concrete proposal:

      • Move exposureId from VisitInfo into ExposureInfo.
      • Add visitId to VisitInfo, and include it as part of makeRawVisitInfo.
      • Document the meaning of exposureId to be clear that it is per-ccd.
      • Document whether visitId can be derived from exposureId, or whether those are decoupled.
      • Document the meaning of visitId to be clear that it is per-visit.

      See this discussion that generated this RFC:

      https://lsstc.slack.com/archives/C3UCAEW3D/p1517724549000034

      Attachments

        Issue Links

          Activity

            rowen Russell Owen added a comment -

            This sounds like a great idea to me. This fixes a long-standing issue in a very natural way.

            I anticipate it will take some obs-package-specific work to set the exposureId field in ExposureInfo and the visitId field in VisitInfo (especially since we presently have no standard way to compute a visit ID from an exposure ID), but it is work that I feel is well worth doing.

            rowen Russell Owen added a comment - This sounds like a great idea to me. This fixes a long-standing issue in a very natural way. I anticipate it will take some obs-package-specific work to set the exposureId field in ExposureInfo and the visitId field in VisitInfo (especially since we presently have no standard way to compute a visit ID from an exposure ID), but it is work that I feel is well worth doing.
            jbosch Jim Bosch added a comment -

            I agree that exposureId should be removed from VisitInfo, and ultimately replaced by an ID corresponding to an actual Visit.

            I am ambivalent about the priority of doing so, and am a bit skeptical that defining a meaningful Visit ID is worth trying to achieve before Gen3.

            A side note: the current schema for Gen3's DataUnits is that Exposure and Visit both exist, but Exposure is not the combination of a Visit and a Sensor.  It is a single observation with a camera (maybe a snap of a raw science visit, maybe a raw calibration image - the latter are not associated into Visits).  We could revisit this, but I think it makes perfect sense if you haven't been corrupted by "exposure" to the afw.image.Exposure class.  The set of people who have been thus corrupted is not even as large as the set of people working for LSST DM, while the set of people who will see these DataUnits should eventually be much larger.  I think we probably cannot live in the long term with Exposure meaning "Visit + Sensor".  My RFC to rename afw.image.Exposure is blocked "only" by the fact that I have no idea what to call it instead.

            jbosch Jim Bosch added a comment - I agree that exposureId  should be removed from VisitInfo , and ultimately replaced by an ID corresponding to an actual Visit . I am ambivalent about the priority of doing so, and am a bit skeptical that defining a meaningful Visit  ID is worth trying to achieve before Gen3. A side note: the current schema for Gen3's DataUnits is that Exposure  and Visit  both exist, but Exposure  is not the combination of a Visit  and a Sensor .  It is a single observation with a camera (maybe a snap of a raw science visit, maybe a raw calibration image - the latter are not associated into Visits ).  We could revisit this, but I think it makes perfect sense if you haven't been corrupted by "exposure" to the  afw.image.Exposure class.  The set of people who have been thus corrupted is not even as large as the set of people working for LSST DM, while the set of people who will see these DataUnits should eventually be much larger.  I think we probably cannot live in the long term with Exposure meaning "Visit + Sensor".  My RFC to rename afw.image.Exposure is blocked "only" by the fact that I have no idea what to call it instead.
            Parejkoj John Parejko added a comment - - edited

            jbosch your comment suggests that there should be two implementation tickets for this: the first one to move ExposureId out of VisitInfo (which can and should be done now, to prevent API breakage in the future), the second to add VisitId to VisitInfo (which should maybe wait for Gen3 Butler).

            > My RFC to rename afw.image.Exposure is blocked "only" by the fact that I have no idea what to call it instead.

            SensorExposure or ChipExposure?

            In which case, the current ExposureId would become SensorExposureId, I guess?

            Parejkoj John Parejko added a comment - - edited jbosch your comment suggests that there should be two implementation tickets for this: the first one to move ExposureId out of VisitInfo (which can and should be done now, to prevent API breakage in the future), the second to add VisitId to VisitInfo (which should maybe wait for Gen3 Butler). > My RFC to rename afw.image.Exposure is blocked "only" by the fact that I have no idea what to call it instead. SensorExposure or ChipExposure? In which case, the current ExposureId would become SensorExposureId , I guess?
            jbosch Jim Bosch added a comment -

            the first one to move ExposureId out of VisitInfo (which can and should be done now, to prevent API breakage in the future), the second to add VisitId to VisitInfo (which should maybe wait for Gen3 Butler).

            Sounds reasonable.

            SensorExposure or ChipExposure?

            Note that we also use this class for coadded images, which is the big reason I dislike the name.

            In which case, the current ExposureId would become SensorExposureId, I guess?

            For raw (science or calibration) images, yes.  For processed images after we've combined snaps, it'd be a VisitSensorId.  It might make more sense to just have a word for the integer ID formed by concatenating the set of DataUnits used to identify that DatasetType (so you don't need a new name for e.g. Jointcal outputs that also need to encode a Tract).  There's no guarantee that stays below 64 bits in general, of course.

            jbosch Jim Bosch added a comment - the first one to move ExposureId out of VisitInfo (which can and should be done now, to prevent API breakage in the future), the second to add VisitId to VisitInfo (which should maybe wait for Gen3 Butler). Sounds reasonable. SensorExposure or ChipExposure? Note that we also use this class for coadded images, which is the big reason I dislike the name. In which case, the current ExposureId would become SensorExposureId, I guess? For raw (science or calibration) images, yes.  For processed images after we've combined snaps, it'd be a VisitSensorId.  It might make more sense to just have a word for the integer ID formed by concatenating the set of DataUnits used to identify that DatasetType (so you don't need a new name for e.g. Jointcal outputs that also need to encode a Tract).  There's no guarantee that stays below 64 bits in general, of course.

            I often want to grab a set of exposures by specifying the visit they all belong to. The proposed change sounds great to me, so long as care is taken to make sure it doesn't break any common use cases of various obs_packages.

            I have no strong opinions about renaming exposure itself, except to note that attaching words like "visit," "sensor," "ccd," and/or "chip" to "exposure" doesn't inherently add clarity about what the object in question is.

            mrawls Meredith Rawls added a comment - I often want to grab a set of exposures by specifying the visit they all belong to. The proposed change sounds great to me, so long as care is taken to make sure it doesn't break any common use cases of various obs_packages. I have no strong opinions about renaming exposure itself, except to note that attaching words like "visit," "sensor," "ccd," and/or "chip" to "exposure" doesn't inherently add clarity about what the object in question is.
            tjenness Tim Jenness added a comment -

            Parejkoj are you ready to adopt this RFC?

            tjenness Tim Jenness added a comment - Parejkoj are you ready to adopt this RFC?
            Parejkoj John Parejko added a comment -

            Adopted, with implementation ticket as a two story epic: DM-13942. The details raised by rowen and jbosch can be decided on as those tickets are implemented.

            I'm going to leave the names as-is here: future name improvements per mrawls's comment can be done in other RFCs.

            Parejkoj John Parejko added a comment - Adopted, with implementation ticket as a two story epic: DM-13942 . The details raised by rowen and jbosch can be decided on as those tickets are implemented. I'm going to leave the names as-is here: future name improvements per mrawls 's comment can be done in other RFCs.

            People

              Parejkoj John Parejko
              Parejkoj John Parejko
              Eli Rykoff, Jim Bosch, John Parejko, John Swinbank, Meredith Rawls, Russell Owen, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                Jenkins

                  No builds found.