Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-28087

obs_lsst accounts for detectors twice in IDs

    XMLWordPrintable

    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

          No builds found.
          ktl Kian-Tat Lim created issue -
          ktl Kian-Tat Lim made changes -
          Field Original Value New Value
          Status To Do [ 10001 ] In Progress [ 3 ]
          Hide
          ktl Kian-Tat Lim added a comment -

          A one-word change. Passes Jenkins on macOS; I don't anticipate problems on CentOS.

          Show
          ktl Kian-Tat Lim added a comment - A one-word change. Passes Jenkins on macOS; I don't anticipate problems on CentOS.
          ktl Kian-Tat Lim made changes -
          Reviewers Jim Bosch [ jbosch ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          Hide
          jbosch Jim Bosch added a comment -

          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.

          Show
          jbosch Jim Bosch added a comment - 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.
          jbosch Jim Bosch made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          Hide
          ktl Kian-Tat Lim added a comment -

          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.

          Show
          ktl Kian-Tat Lim added a comment - 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.
          Hide
          jbosch Jim Bosch added a comment -

          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.

          Show
          jbosch Jim Bosch added a comment - 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.
          ktl Kian-Tat Lim made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]

            People

            Assignee:
            ktl Kian-Tat Lim
            Reporter:
            ktl Kian-Tat Lim
            Reviewers:
            Jim Bosch
            Watchers:
            Jim Bosch, Kian-Tat Lim, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.