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

Allow depersist of old Calib objects as PhotoCalibs

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      For backwards compatibility, we will want to be able to de-persist data processed with old stacks that have {{Calib}}s in them, and produce a PhotoCalib. We should be able to keep the old CalibFactory object and its registration and have it produce a PhotoCalib instead.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment - - edited

            Jim Bosch: are you still able to review this ticket? It's about 300 lines now, since I added versioning to ExposureInfo FITS persistence as part of it (in preparation for the Calib->PhotoCalib transition).

            Here's the PR, since jira isn't picking it up: https://github.com/lsst/afw/pull/435

            Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29417/pipeline

            Show
            Parejkoj John Parejko added a comment - - edited Jim Bosch : are you still able to review this ticket? It's about 300 lines now, since I added versioning to ExposureInfo FITS persistence as part of it (in preparation for the Calib->PhotoCalib transition). Here's the PR, since jira isn't picking it up: https://github.com/lsst/afw/pull/435 Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29417/pipeline
            Hide
            jbosch Jim Bosch added a comment -

            Code looks good (comments on PR), but I don't actually see anything to address the stated goal of this ticket (which involves modifying the afw::table::io persistence for Calib, not just the FITS metadata code path).  Was that moved to DM-10156, because it can't reasonably be done before actually removing Calib from Exposure?

            Show
            jbosch Jim Bosch added a comment - Code looks good (comments on PR), but I don't actually see anything to address the stated goal of this ticket (which involves modifying the afw::table::io persistence for Calib, not just the FITS metadata code path).  Was that moved to DM-10156 , because it can't reasonably be done before actually removing Calib from Exposure?
            Hide
            Parejkoj John Parejko added a comment -

            I don't actually see anything to address the stated goal of this ticket.

            I don't know whether there is more that we can do before the actual transition off of Calib. If there is something else that I can implement here, before we replace Calib with PhotoCalib everywhere, please tell me! e.g., the afw::table::io persistence we probably can't, but should I modify one of the various registered Factories, or is there another code path I need to prep?

            Show
            Parejkoj John Parejko added a comment - I don't actually see anything to address the stated goal of this ticket. I don't know whether there is more that we can do before the actual transition off of Calib. If there is something else that I can implement here, before we replace Calib with PhotoCalib everywhere, please tell me! e.g., the afw::table::io persistence we probably can't, but should I modify one of the various registered Factories, or is there another code path I need to prep?
            Hide
            jbosch Jim Bosch added a comment -

            Maybe.  We will eventually need a copy of CalibFactory (Calib.cc: 401, at least on master) that returns a PhotoCalib instead of a Calib but accepts the same inputs.  We can't register that now, though, and that would make it very tricky to test right now, as we'd have to mock up a slightly-modified version of a lot of the persistence code to call it with the kinds of inputs it would expect.  Once we are ready to do the replacement, we could just register that factory instead of the current CalibFactory and test it by trying to read a Calib saved to an ExposureCatalog (trying to read a Calib from an old Exposure should go through that code path too, but since that should fall back to using the metadata if it fails it's not as clean of a test).

            Show
            jbosch Jim Bosch added a comment - Maybe.  We will eventually need a copy of CalibFactory (Calib.cc: 401, at least on master) that returns a PhotoCalib instead of a Calib but accepts the same inputs.  We can't register that now, though, and that would make it very tricky to test right now, as we'd have to mock up a slightly-modified version of a lot of the persistence code to call it with the kinds of inputs it would expect.  Once we are ready to do the replacement, we could just register that factory instead of the current CalibFactory and test it by trying to read a Calib saved to an ExposureCatalog (trying to read a Calib from an old Exposure should go through that code path too, but since that should fall back to using the metadata if it fails it's not as clean of a test).
            Hide
            Parejkoj John Parejko added a comment -

            Thank you for the review, and for the C++ lesson. I switched makePhotoCalib(metadata) to return nullptr if the metadata keys weren't available, instead of throwing.

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thank you for the review, and for the C++ lesson. I switched makePhotoCalib(metadata) to return nullptr if the metadata keys weren't available, instead of throwing. Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Jim Bosch
                Watchers:
                Jim Bosch, John Parejko, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel