Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: obs_lsst
-
Labels:None
-
Team:Architecture
-
Urgent?:No
Description
https://github.com/lsst/obs_lsst/blob/master/python/lsst/obs/lsst/_instrument.py#L117-L128 uses the max_detector_exposure_id as the maximum value of the visit and observation dimensions. But those maximums are later multiplied again by the maximum detector count. I believe the max_exposure_id should be used instead.
Attachments
Activity
| Field | Original Value | New Value |
|---|---|---|
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
| Reviewers | Jim Bosch [ jbosch ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
Looks good. We will have to create new repos to make sure of this change, and I am a bit worried that exposure_max == visit_max remains a bit naive. But this is certainly a step in the right direction, and as we discussed on slack, we might need to shrink these anyway if we want Source IDs to fit in 64 bits.
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
I didn't think about backwards compatibility. Using this code with old repos could cause problems decoding existing IDs, even if new ones are OK? That would mean that this is actually an interface-breaking change.
This should be completely safe to use with old repos. The bug will just not be fixed in old repos, and the packed IDs generated by code run against old repos will be inconsistent with the packed IDs generated by code run against new repos.
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
A one-word change. Passes Jenkins on macOS; I don't anticipate problems on CentOS.