Details
-
Type:
Bug
-
Status: Invalid
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:
Description
exposureId 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. exposureId should be moved back into ExposureInfo.
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']). We could have makeRawVisitInfo() add the butler visitId to the new visitInfo.visitId that this ticket will add.
Marking this Critical because we should fix this ASAP, before use of visitInfo.exposureId becomes widespread. This is an API-breaking change, so it may need an RFC?
See this discussion that generated this ticket:
https://lsstc.slack.com/archives/C3UCAEW3D/p1517724549000034
Attachments
Issue Links
Activity
| Field | Original Value | New Value |
|---|---|---|
| Description |
[{exposureId}} 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. {{exposureId}} should be moved back into {{ExposureInfo}}.
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']}}). We could have {{makeRawVisitInfo()}} add the butler visitId to the new {{visitInfo.visitId}} that this ticket will add. Marking this Critical because we should fix this ASAP, before use of {{visitInfo.exposureId}} becomes widespread. This is an API-breaking change, so it may need an RFC? See this discussion that generated this ticket: https://lsstc.slack.com/archives/C3UCAEW3D/p1517724549000034 |
{{exposureId}} 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. {{exposureId}} should be moved back into {{ExposureInfo}}.
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']}}). We could have {{makeRawVisitInfo()}} add the butler visitId to the new {{visitInfo.visitId}} that this ticket will add. Marking this Critical because we should fix this ASAP, before use of {{visitInfo.exposureId}} becomes widespread. This is an API-breaking change, so it may need an RFC? See this discussion that generated this ticket: https://lsstc.slack.com/archives/C3UCAEW3D/p1517724549000034 |
| Comment | [ I think we need a field like this in VisitInfo, though I agree that it is not being set correctly at this time. I suggest we retain it, but change our code to not set it until we can set it properly. Furthermore, we can probably modify most obs packages to set it correctly without much difficulty. A standardized, non-obs-specific means of setting it will have to wait, but meanwhile we can have a proper visit ID. ] |
Thank you for the RFC. Well done.
The RFC has been adopted. John Parejko, do you want to claim credit for this and mark it done?
The work hasn't been done. RFCs in adopted state are RFCs where we have agreed to do the work but the work is still to be done. Implemented means that the work has been completed.
The RFC has an epic with two additional tickets to implement it. That would seem to make this ticket redundant.
It could be argued that this ticket is a duplicate of DM-13943. Maybe John Parejko wants to comment since he filed this.
Marking this as a duplicate of DM-13943: I think that ticket has the better approach (moving it out of VisitInfo and into ExposureInfo, where it belongs). Doing that will take care of this.
| Resolution | Done [ 10000 ] | |
| Status | To Do [ 10001 ] | Invalid [ 11005 ] |
Krzysztof Findeisen
made changes -
(a) I wholeheartedly endorse this, but (b) I'm pretty sure it requires an RFC.