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

Convert meas_mosaic wcs output to a format directly readable by the butler

    Details

    • Story Points:
      2
    • Sprint:
      DRP S18-3
    • Team:
      Data Release Production

      Description

      meas_mosaic writes its WCS as FITS header with no attached image, which requires loading code to use the following pattern:

      md = butler.get("wcs_md", ...)
      wcs = lsst.afw.image.makeWcs(md)
      

      instead of simply

      wcs = butler.get("wcs", ...)
      

      Using a header-only format also limits us to FITS-standard WCS mappings.

      Because Wcs inherits from afw::table::io::Persistable it already has writeFits and readFits methods that utilize our FITS binary table format, which will be able to save more complex WCS solutions. It's also compatible (or will be soon, on DM-10728) with the "FitsCatalogStorage" butler storage type, so we should be able to fix this by:

      • Redefining "wcs" to be a "FitsCatalogStorage" dataset, instead of a "FitsStorage" exposure in all mappers;
      • modifying meas_mosaic (and possibly jointcal, if needed) to use butler.put directly.
      • modifying any code that consumes the wcs dataset to use butler.get directly.

      In addition, this issue should include creating a simple command-line script that can be used to convert a data repository from the old format to the new one.

        Attachments

          Issue Links

            Activity

            jbosch Jim Bosch created issue -
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Watchers Jim Bosch, John Parejko, Paul Price [ Jim Bosch, John Parejko, Paul Price ] Jim Bosch, John Parejko, Paul Price, Russell Owen [ Jim Bosch, John Parejko, Paul Price, Russell Owen ]
            jbosch Jim Bosch made changes -
            Link This issue is triggered by RFC-450 [ RFC-450 ]
            jbosch Jim Bosch made changes -
            Epic Link DM-12729 [ 36328 ]
            jbosch Jim Bosch made changes -
            Component/s obs_base [ 10719 ]
            Component/s obs_subaru [ 10747 ]
            Sprint DRP S18-3 [ 687 ]
            Team Data Release Production [ 10301 ]
            Assignee Jim Bosch [ jbosch ]
            jbosch Jim Bosch made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            Parejkoj John Parejko added a comment - - edited

            This will also require changes to validate_drp. (edit: just did it)

            Show
            Parejkoj John Parejko added a comment - - edited This will also require changes to validate_drp . (edit: just did it)
            jbosch Jim Bosch made changes -
            Component/s validate_drp [ 14011 ]
            jbosch Jim Bosch made changes -
            Component/s obs_cfht [ 10762 ]
            Component/s obs_decam [ 12851 ]
            Component/s validate_drp [ 14011 ]
            jbosch Jim Bosch made changes -
            Component/s pipe_tasks [ 10726 ]
            Hide
            jbosch Jim Bosch added a comment -

            I've made what I believe are the necessary changes for obs_* and meas_mosaic.  Looks like pipe_tasks does not need any changes; it just passes a DataRef to meas_mosaic code.  I'll probably put off testing until next week so I can just run on top of a weekly build instead of rebuilding a stack with the SkyWcs changes now. 

            Show
            jbosch Jim Bosch added a comment - I've made what I believe are the necessary changes for obs_* and meas_mosaic.  Looks like pipe_tasks does not need any changes; it just passes a DataRef to meas_mosaic code.  I'll probably put off testing until next week so I can just run on top of a weekly build instead of rebuilding a stack with the SkyWcs changes now. 
            jbosch Jim Bosch made changes -
            Link This issue blocks DM-13579 [ DM-13579 ]
            Parejkoj John Parejko made changes -
            Link This issue relates to DM-12796 [ DM-12796 ]
            Hide
            jbosch Jim Bosch added a comment -

            Lauren MacArthur, could you review this set of trivial (mostly naming) changes?

            I also suspect there might be code in pipe_analysis that would need to be changed as well; I have not attempted to do that yet.

            Show
            jbosch Jim Bosch added a comment - Lauren MacArthur , could you review this set of trivial (mostly naming) changes? I also suspect there might be code in pipe_analysis that would need to be changed as well; I have not attempted to do that yet.
            jbosch Jim Bosch made changes -
            Reviewers Lauren MacArthur [ lauren ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            lauren Lauren MacArthur added a comment -

            Looks good Jim.  Some minor comments on the PR. Has this been pushed through some data (Jenkins won't cut it for the `meas_mosaic` testing...I assume `jointcal` has unittests that would catch errors)?
             
            Also, the description states:

            In addition, this issue should include creating a simple command-line script that can be used to convert a data repository from the old format to the new one.

            Are you punting on that? I think you included enough backward-compatibility such that this is not strictly necessary...but it would probably mean a longer delay before being able to remove the backward-compatible code if that is desired.

            Show
            lauren Lauren MacArthur added a comment - Looks good Jim.  Some minor comments on the PR. Has this been pushed through some data (Jenkins won't cut it for the `meas_mosaic` testing...I assume `jointcal` has unittests that would catch errors)?   Also, the description states: In addition, this issue should include creating a simple command-line script that can be used to convert a data repository from the old format to the new one. Are you punting on that? I think you included enough backward-compatibility such that this is not strictly necessary...but it would probably mean a longer delay before being able to remove the backward-compatible code if that is desired.
            lauren Lauren MacArthur made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            lauren Lauren MacArthur added a comment -

            Oh, and I think pipe_analysis is fine as is (it only checks on datasetExists, the "getting" is delegated to applyMosaicResultsCatalog in lsst.meas.mosaic.updateExposure).

            Show
            lauren Lauren MacArthur added a comment - Oh, and I think pipe_analysis is fine as is (it only checks on datasetExists, the "getting" is delegated to applyMosaicResultsCatalog in lsst.meas.mosaic.updateExposure ).
            Hide
            jbosch Jim Bosch added a comment -

            Yes, I think the backwards-compatibility code in meas_mosaic removes the need for the script to convert data repositories.

             

            In addition to Jenkins, I'll rerun meas_mosaic on the next weekly outputs and remake at least one coadd patch; I think if that goes through, it should be sufficient to say that this works (hard to imagine this change breaking something without it resulting in a hard failure).

            Show
            jbosch Jim Bosch added a comment - Yes, I think the backwards-compatibility code in meas_mosaic removes the need for the script to convert data repositories.   In addition to Jenkins, I'll rerun meas_mosaic on the next weekly outputs and remake at least one coadd patch; I think if that goes through, it should be sufficient to say that this works (hard to imagine this change breaking something without it resulting in a hard failure).
            Hide
            jbosch Jim Bosch added a comment -

            I'm going to defer the final test and merge of this to next week - since the last weekly, DM-12447 introduced an ABI change in afw, and that means that I can't do the meas_mosaic tests without either rebuilding essentially the full stack on lsst-dev or just waiting for the next weekly.  The latter is a lot easier.

            Show
            jbosch Jim Bosch added a comment - I'm going to defer the final test and merge of this to next week - since the last weekly, DM-12447 introduced an ABI change in afw, and that means that I can't do the meas_mosaic tests without either rebuilding essentially the full stack on lsst-dev or just waiting for the next weekly.  The latter is a lot easier.
            Hide
            jbosch Jim Bosch added a comment -

            meas_mosaic tests mentioned have been completed; I re-ran meas_mosaic and then rebuilt a single-patch coadd.  The pixel values were identical to the same patch in the w_2018_08/DM-13532 run, and I think that's sufficient.

            Running one more Jenkins job, as I think I needed to rebase some packages since the last one.

            Show
            jbosch Jim Bosch added a comment - meas_mosaic tests mentioned have been completed; I re-ran meas_mosaic and then rebuilt a single-patch coadd.  The pixel values were identical to the same patch in the w_2018_08/ DM-13532 run, and I think that's sufficient. Running one more Jenkins job, as I think I needed to rebase some packages since the last one.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master.

            Show
            jbosch Jim Bosch added a comment - Merged to master.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            hchiang2 Hsin-Fang Chiang made changes -
            Link This issue is triggering DM-13760 [ DM-13760 ]
            yusra Yusra AlSayyad made changes -
            Story Points 2
            yusra Yusra AlSayyad made changes -
            Risk Score 0

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Lauren MacArthur
                Watchers:
                Hsin-Fang Chiang, Jim Bosch, John Parejko, Lauren MacArthur, Paul Price, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel