Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-10007 Provide Jointcal requirements document
  3. DM-10728

Near-term jointcal acceptance: make jointcal and meas_mosaic use the same output formats

    Details

    • Type: Technical task
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw, meas_mosaic
    • Labels:
      None
    • Sprint:
      DRP S17-6, DRP F17-1, DRP F17-2, DRP F17-3, DRP F17-4
    • Team:
      Data Release Production

      Description

      Have meas_mosaic use an output Butler dataset and file format that could be used by jointcal in the future, allowing jointcal to be a true "drop-in replacement" for meas_mosaic. I intend to keep the current "wcs" dataset for astrometry, and add a new "photoCalib" dataset (saved directly as an afw.image.PhotoCalib instance). Implementing this will require adding a BoundedField wrapper for for meas_mosaic's photometric output.

        Attachments

          Issue Links

            Activity

            Hide
            lauren Lauren MacArthur added a comment -

            Ok, I am now convinced that all is in order. I admit I am in no position to review any of the C++ code, but it looks ok as far as I can tell. In order to prove to myself that the photoCalib is indeed persisting a fit equivalent to the the original meas_mosaic fcr fits, I used your CorrectionImageSource to display the persisted fcr fits using cameraGeom's showCamera (after a small correction to your class, see below). I also wrote a PhotoCalibImageSource class to similarly display the persisted photoCalib fit. Here they are side-by-side:

            I matched the scales and blinked them to be sure they are effectively (i.e. within tolerance of ~E-6) identical. I also zoomed in on the rotated CCDs to be absolutely sure.

            I've pushed a u/lauren/DM-10728 branch to meas_mosaic with my changes and additions. I committed them as fixup's where appropriate so you should be able to rebase them out if you choose to include them. The only things you a required to use are:

            You should probably also double-check my PhotoCalibImageSource if you choose to include it:
            https://github.com/lsst/meas_mosaic/commit/8695246375b65b0b6272e443b4abc2ac4dae54bb

            Oh, and as you are aware, I pushed a new tickets/DM-10728 branch to obs_subaru (the dataset definition was in the wrong place: I moved it from exposures: to datasets:).

            I have put a few other comments on the PR so, with the caveat that I have not scrutinized the C++ code, I consider this review complete.

            Show
            lauren Lauren MacArthur added a comment - Ok, I am now convinced that all is in order. I admit I am in no position to review any of the C++ code, but it looks ok as far as I can tell. In order to prove to myself that the photoCalib is indeed persisting a fit equivalent to the the original meas_mosaic fcr fits, I used your CorrectionImageSource to display the persisted fcr fits using cameraGeom's showCamera (after a small correction to your class, see below). I also wrote a PhotoCalibImageSource class to similarly display the persisted photoCalib fit. Here they are side-by-side: I matched the scales and blinked them to be sure they are effectively (i.e. within tolerance of ~E-6) identical. I also zoomed in on the rotated CCDs to be absolutely sure. I've pushed a u/lauren/ DM-10728 branch to meas_mosaic with my changes and additions. I committed them as fixup 's where appropriate so you should be able to rebase them out if you choose to include them. The only things you a required to use are: https://github.com/lsst/meas_mosaic/commit/027f70d2193bdbd43bb5eaaa6f30bf3897ef34e4 (modulo my extra comments if you feel they are unhelpful) MaskU -> Mask here: https://github.com/lsst/meas_mosaic/commit/0155fcebcf54989924e61f1adde67a8c22ab82f0#diff-98a57e20f4598e5dbb5fbce3ddfbfe35R44 You should probably also double-check my PhotoCalibImageSource if you choose to include it: https://github.com/lsst/meas_mosaic/commit/8695246375b65b0b6272e443b4abc2ac4dae54bb Oh, and as you are aware, I pushed a new tickets/ DM-10728 branch to obs_subaru (the dataset definition was in the wrong place: I moved it from exposures: to datasets: ). I have put a few other comments on the PR so, with the caveat that I have not scrutinized the C++ code, I consider this review complete.
            Hide
            jbosch Jim Bosch added a comment -

            Thanks for all of the fixes, Lauren MacArthur! I've now integrated everything on u/lauren/DM-10728 into tickets/DM-10728, and I believe I've addressed all review comments. Will merge once I have a successful Jenkins run with the rebased branches.

            John Parejko, I haven't yet gotten far enough in the review of DM-11095 to know if my changes to PhotoCalib here will disrupt anything you're doing in jointcal there (I did see one commit message that made me think they might). If they do, I'll give you a jointcal commit to fix things (which you can then revert when RFC-332 is implemented along with my PhotoCalib changes).

            Show
            jbosch Jim Bosch added a comment - Thanks for all of the fixes, Lauren MacArthur ! I've now integrated everything on u/lauren/ DM-10728 into tickets/ DM-10728 , and I believe I've addressed all review comments. Will merge once I have a successful Jenkins run with the rebased branches. John Parejko , I haven't yet gotten far enough in the review of DM-11095 to know if my changes to PhotoCalib here will disrupt anything you're doing in jointcal there (I did see one commit message that made me think they might). If they do, I'll give you a jointcal commit to fix things (which you can then revert when RFC-332 is implemented along with my PhotoCalib changes).
            Hide
            lauren Lauren MacArthur added a comment -

            Jenkins won't exercise meas_mosaic, so it's probably worth running it one last time with all the rebased branches. Let me know if you'd like me to do that

            Show
            lauren Lauren MacArthur added a comment - Jenkins won't exercise meas_mosaic , so it's probably worth running it one last time with all the rebased branches. Let me know if you'd like me to do that
            Hide
            jbosch Jim Bosch added a comment -

            I'll give it a try, unless I can't get it to build against the latest weekly now that it's all be rebased on master. If that's the case, I may just merge everything but meas_mosaic now and wait for the next weekly to test and merge it, since none of the other changes depend on meas_mosaic.

            Show
            jbosch Jim Bosch added a comment - I'll give it a try, unless I can't get it to build against the latest weekly now that it's all be rebased on master. If that's the case, I may just merge everything but meas_mosaic now and wait for the next weekly to test and merge it, since none of the other changes depend on meas_mosaic.
            Hide
            jbosch Jim Bosch added a comment -

            meas_mosaic and Jenkins runs successful; merged to master.

            Show
            jbosch Jim Bosch added a comment - meas_mosaic and Jenkins runs successful; merged to master.

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                John Parejko, Lauren MacArthur, Nate Pease
                Watchers:
                Jim Bosch, John Parejko, Lauren MacArthur, Nate Pease
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel