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

Implement new initial WCS design

    XMLWordPrintable

    Details

    • Story Points:
      8
    • Epic Link:
    • Sprint:
      AP S19-6, AP F19-1, AP F19-2
    • Team:
      Alert Production

      Description

      The close of DM-19951 describes a new design for how we create an initial WCS from the raw exposure VisitInfo metadata. This ticket is to implement that design. In summary:

      • Write tests against sensor bounding boxes on known data (e.g. testdata_*).
      • Write a new function to create a SkyWcs from VisitInfo and Detector (createInitialSkyWcs?).
      • Call that function from exposureFromImage.
      • Reorder the reading of raw VisitInfo and WCS metadata to deal potential metadata stripping problems.
      • Check that all existing cameras use this code path, and not some custom Exposure metadata handling.
      • Turn off addDistortionModel in ISR for all cameras, so that the above WCS does not get overwritten.

        Attachments

          Issue Links

            Activity

            No builds found.
            Parejkoj John Parejko created issue -
            Parejkoj John Parejko made changes -
            Field Original Value New Value
            Epic Link DM-19943 [ 306001 ]
            Parejkoj John Parejko made changes -
            Link This issue is triggered by DM-19951 [ DM-19951 ]
            Parejkoj John Parejko made changes -
            Component/s obs_base [ 10719 ]
            Sprint AP S19-6 [ 834 ]
            Story Points 8
            Team Alert Production [ 10300 ]
            Assignee John Parejko [ parejkoj ]
            Description The close of DM-19951 describes a new design for how we create an initial WCS from the raw exposure VisitInfo metadata. This ticket is to implement that design. In summary:

            * Write tests against sensor bounding boxes on known data (e.g. {{testdata_*}}).
            * Write a new function to create a SkyWcs from VisitInfo and Detector ({{createInitialSkyWcs}}?).
            * Call that function from {{exposureFromImage}}.
            * Reorder the reading of raw VisitInfo and WCS metadata to deal potential metadata stripping problems.
            * Check that all existing cameras use this code path, and not some custom Exposure metadata handling.
            * Turn off {{addDistortionModel}} in ISR for all cameras, so that the above WCS does not get overwritten.
            Labels SciencePipelines
            Summary obs_base Implement new initial WCS design
            Parejkoj John Parejko made changes -
            Watchers John Parejko [ John Parejko ] Chris Morrison, Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur [ Chris Morrison, Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur ]
            swinbank John Swinbank made changes -
            Sprint AP S19-6 [ 834 ] AP S19-6, AP F19-1 [ 834, 923 ]
            Parejkoj John Parejko made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            Parejkoj John Parejko added a comment -

            As an initial test, we should try to write a VisitInfo/Detector wrapper around the appropriate makeSkyWcs (something like what createInitialSkyWcs will look like) and use it in a notebook with DECam/CFHT to see if they behave nicely.

            Show
            Parejkoj John Parejko added a comment - As an initial test, we should try to write a VisitInfo/Detector wrapper around the appropriate makeSkyWcs (something like what createInitialSkyWcs will look like) and use it in a notebook with DECam/CFHT to see if they behave nicely.
            jbosch Jim Bosch made changes -
            Link This issue relates to DM-20188 [ DM-20188 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20202 [ DM-20202 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20201 [ DM-20201 ]
            Hide
            Parejkoj John Parejko added a comment -

            Progress report on this: I've tweaked the decam and hsc obs packages to work with the obs_base updates, and stripped out some code from IsrTask. I'm running a stack build with those to see what happens further down the line. I expect that processCcd may have problems with the new WCSs, if the testdata boresights are not great. I think it's in a state where someone could try testing it in the manner described in the first "How to test" header in this comment, which is similar to the exploration that Chris Morrison [X] did in DM-20188, except now running processCcd on it.

            Show
            Parejkoj John Parejko added a comment - Progress report on this: I've tweaked the decam and hsc obs packages to work with the obs_base updates, and stripped out some code from IsrTask . I'm running a stack build with those to see what happens further down the line. I expect that processCcd may have problems with the new WCSs, if the testdata boresights are not great. I think it's in a state where someone could try testing it in the manner described in the first "How to test" header in this comment , which is similar to the exploration that Chris Morrison [X] did in DM-20188 , except now running processCcd on it.
            Hide
            Parejkoj John Parejko added a comment -

            I've got things building through lsst_distrib now, but ci_hsc is failing because the initial HSC SkyWcs is incorrect. I tried to fix the plateScale in the hsc CameraGeom, but either I'm not getting it right, or there are deeper problems than just changing config.plateScale (and commenting out the two other plateScale statements below). I'll be the coefficients of the distortion model depend on plate scale being 1?

            Show
            Parejkoj John Parejko added a comment - I've got things building through lsst_distrib now, but ci_hsc is failing because the initial HSC SkyWcs is incorrect. I tried to fix the plateScale in the hsc CameraGeom, but either I'm not getting it right, or there are deeper problems than just changing config.plateScale (and commenting out the two other plateScale statements below). I'll be the coefficients of the distortion model depend on plate scale being 1?
            Hide
            Parejkoj John Parejko added a comment -

            Now that I've gotten ci_hsc to pass on this ticket, any other suggestions about testing? I'd like to do at least one more test of some sort before putting this in review.

            Chris Morrison [X] and Lauren MacArthur: You should be able to build a stack based on this branch do whatever tests or further development you need.

            Show
            Parejkoj John Parejko added a comment - Now that I've gotten ci_hsc to pass on this ticket, any other suggestions about testing? I'd like to do at least one more test of some sort before putting this in review. Chris Morrison [X] and Lauren MacArthur : You should be able to build a stack based on this branch do whatever tests or further development you need.
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20373 [ DM-20373 ]
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-20378 [ DM-20378 ]
            Show
            Parejkoj John Parejko added a comment - - edited Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30061/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Jim Bosch: can you please review these two gen3-related commits, before I put the whole thing into a full review, to check that this follows your intention for how gen3 behaves? I'm worried that we don't have any unittests for this code: I just fixed one bug in it that should have been caught by some simple mocks. Is there a reason I shouldn't add any?

            https://github.com/lsst/obs_base/pull/156/commits/4881340cc6084825b819f6118d20bb5ff1c39941
            https://github.com/lsst/obs_subaru/pull/213/commits/f0c563f5f5b1cb90793f2f96838a86ca6f674e21

            Show
            Parejkoj John Parejko added a comment - Jim Bosch : can you please review these two gen3-related commits, before I put the whole thing into a full review, to check that this follows your intention for how gen3 behaves? I'm worried that we don't have any unittests for this code: I just fixed one bug in it that should have been caught by some simple mocks. Is there a reason I shouldn't add any? https://github.com/lsst/obs_base/pull/156/commits/4881340cc6084825b819f6118d20bb5ff1c39941 https://github.com/lsst/obs_subaru/pull/213/commits/f0c563f5f5b1cb90793f2f96838a86ca6f674e21
            Parejkoj John Parejko made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment -

            I've left some comments on the obs_base PR, mostly about ways we might want to give obs packages more control over the degree to which an boresight+cameraGeom WCS is expected and/or required.  I don't think those need to spur any changes, but I think it's worth discussing.

            Show
            jbosch Jim Bosch added a comment - I've left some comments on the obs_base PR, mostly about ways we might want to give obs packages more control over the degree to which an boresight+cameraGeom WCS is expected and/or required.  I don't think those need to spur any changes, but I think it's worth discussing.
            Parejkoj John Parejko made changes -
            Link This issue is triggering DM-20499 [ DM-20499 ]
            Parejkoj John Parejko made changes -
            Reviewers Jim Bosch [ jbosch ] Meredith Rawls [ mrawls ]
            Hide
            Parejkoj John Parejko added a comment -

            Meredith Rawls: thank you for agreeing to review this. There are 5 PRs to review, the largest in obs_base. This ticket implements most of the design described in the final comment of DM-19951. See DM-20188 for some of the tests we did of various cameras, looking at the difference between the pure FITS WCS from the raw files, the fitted calexp WCS, the boresight+distortion model WCS created by an early version of this code (summary: this code gets you quite close to the calexp WCS, modulo a shift/rotation).

            Show
            Parejkoj John Parejko added a comment - Meredith Rawls : thank you for agreeing to review this. There are 5 PRs to review, the largest in obs_base. This ticket implements most of the design described in the final comment of DM-19951 . See DM-20188 for some of the tests we did of various cameras, looking at the difference between the pure FITS WCS from the raw files, the fitted calexp WCS, the boresight+distortion model WCS created by an early version of this code (summary: this code gets you quite close to the calexp WCS, modulo a shift/rotation).
            Parejkoj John Parejko made changes -
            Status In Review [ 10004 ] In Progress [ 3 ]
            Hide
            Parejkoj John Parejko added a comment -

            Meredith Rawls: I'm pulling this out of review for the moment: there are a couple of RFCs that will have to go through first, which will likely cause some rebasing. Once that's all done, I'll request a review again.

            Show
            Parejkoj John Parejko added a comment - Meredith Rawls : I'm pulling this out of review for the moment: there are a couple of RFCs that will have to go through first, which will likely cause some rebasing. Once that's all done, I'll request a review again.
            swinbank John Swinbank made changes -
            Sprint AP S19-6, AP F19-1 [ 834, 923 ] AP S19-6, AP F19-1, AP F19-2 [ 834, 923, 940 ]
            swinbank John Swinbank made changes -
            Link This issue is triggered by RFC-616 [ RFC-616 ]
            Parejkoj John Parejko made changes -
            Link This issue has to be finished together with DM-20742 [ DM-20742 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20746 [ DM-20746 ]
            swinbank John Swinbank made changes -
            Link This issue has to be finished together with DM-20742 [ DM-20742 ]
            Hide
            Parejkoj John Parejko added a comment -

            Ok, finally back in review now, Meredith Rawls. There are 5 PRs: on the obs_subaru PR, only look at my two commits (I've rebased it onto Lauren MacArthur's work redoing the HSC plate scale, but that will be reviewed separately).

            Show
            Parejkoj John Parejko added a comment - Ok, finally back in review now, Meredith Rawls . There are 5 PRs: on the obs_subaru PR, only look at my two commits (I've rebased it onto Lauren MacArthur 's work redoing the HSC plate scale, but that will be reviewed separately).
            Parejkoj John Parejko made changes -
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            mrawls Meredith Rawls added a comment -

            After lots of fun discussions about docstring inheritance, CRVAL being the actual worst, deprecation warnings, etc., I'm marking this as reviewed pending an ap_verify run on the CI dataset demonstrating the metrics that do not wildly differ from a recent ap_verify actual-CI run. Let me know if you want help with that!

            Show
            mrawls Meredith Rawls added a comment - After lots of fun discussions about docstring inheritance, CRVAL being the actual worst, deprecation warnings, etc., I'm marking this as reviewed pending an ap_verify run on the CI dataset demonstrating the metrics that do not wildly differ from a recent ap_verify actual-CI run. Let me know if you want help with that!
            mrawls Meredith Rawls made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20548 [ DM-20548 ]
            Hide
            Parejkoj John Parejko added a comment - - edited

            I ran ap_verify on ap_verify_ci_hits2015, and the output seems to match that of past runs (looking at the dashboard and one run against master, I get 1706 unassociated sources), so I think we're ok there.

            I'm still blocked on DM-20548, but otherwise I think we're ok to merge.

            Show
            Parejkoj John Parejko added a comment - - edited I ran ap_verify on ap_verify_ci_hits2015 , and the output seems to match that of past runs (looking at the dashboard and one run against master, I get 1706 unassociated sources), so I think we're ok there. I'm still blocked on DM-20548 , but otherwise I think we're ok to merge.
            Parejkoj John Parejko made changes -
            Link This issue is blocked by DM-20746 [ DM-20746 ]
            Parejkoj John Parejko made changes -
            Link This issue relates to DM-20746 [ DM-20746 ]
            Parejkoj John Parejko made changes -
            Link This issue relates to DM-20937 [ DM-20937 ]
            Show
            Parejkoj John Parejko added a comment - Post-rebase Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30312/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            After rebasing, ap_verify_ci_hits2015 still produces the same metric, so we're still ok there. DM-20548 is merged, so once the above Jenkins run completes successfully, I'll finally merge this.

            Show
            Parejkoj John Parejko added a comment - After rebasing, ap_verify_ci_hits2015 still produces the same metric, so we're still ok there. DM-20548 is merged, so once the above Jenkins run completes successfully, I'll finally merge this.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for all the review comments and testing suggestions Meredith Rawls.

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thanks for all the review comments and testing suggestions Meredith Rawls . Merged and done.
            Parejkoj John Parejko made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            erykoff Eli Rykoff made changes -
            Link This issue relates to DM-21248 [ DM-21248 ]

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Meredith Rawls
              Watchers:
              Chris Morrison, Jim Bosch, John Parejko, John Swinbank, Lauren MacArthur, Meredith Rawls
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.