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

Investigate change in fracDiaSourcesToSciSources in ap_verify CI

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ap_verify, obs_decam
    • Labels:
      None
    • Story Points:
      1
    • Epic Link:
    • Sprint:
      AP S20-5 (April)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      Between 26 & 27 March, the fracDiaSourcesToSciSources (“Ratio of DIASources to Direct Image Sources per CCD”) being tracked by Chronograf very ap_verify_ci_hits2015 changed for unknown reasons. What happened?

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            This was discussed at the AP Pipeline Meeting of 2020-04-06. At that meeting, we theorized that this commit on DM-23616 might be responsible.

            In fact, the change does come from DM-23616, but not that commit. Rather, DM-23616 had the side effect of changing config.ccdProcessor.calibrate.astrometry.wcsFitter.order from 2 to 4.

            In more detail: obs_decam contains files config/processCcd.py and config/processCcdCpIsr.py, where the latter is now obsolete but used to be specific configuration for using cp calibration products. ap_verify_ci_hits2015 used to refer to the latter, but now refers to the former. processCcd.py sets calibrate.wcsFitter.order=4 (actually, it used to do this directly, but now delegates to calibrate.py which does it instead). processCcdCpIsr.py did not (although now it does, since it now delegates to processCcd.py, which, in turn, delegates to calibrate.py).

            Next question obviously is: what should this be set to? RFC-577 reduced it to 2 by default. Despite a comment from John Parejko on that ticket that “a 2nd order fit is probably what we want”, DM-18293 implemented RFC-577 by overriding that default to 4 for obs_decam (except not in the case of CP calibs, for the reasons described above). So DM-23616 effectively enabled an earlier override that had been inadvertently disabled.

            Show
            swinbank John Swinbank added a comment - This was discussed at the AP Pipeline Meeting of 2020-04-06 . At that meeting, we theorized that this commit on DM-23616 might be responsible. In fact, the change does come from DM-23616 , but not that commit. Rather, DM-23616 had the side effect of changing config.ccdProcessor.calibrate.astrometry.wcsFitter.order from 2 to 4. In more detail: obs_decam contains files config/processCcd.py and config/processCcdCpIsr.py , where the latter is now obsolete but used to be specific configuration for using cp calibration products. ap_verify_ci_hits2015 used to refer to the latter, but now refers to the former . processCcd.py sets calibrate.wcsFitter.order=4 (actually, it used to do this directly, but now delegates to calibrate.py which does it instead). processCcdCpIsr.py did not (although now it does, since it now delegates to processCcd.py , which, in turn, delegates to calibrate.py ). Next question obviously is: what should this be set to? RFC-577 reduced it to 2 by default. Despite a comment from John Parejko on that ticket that “a 2nd order fit is probably what we want”, DM-18293 implemented RFC-577 by overriding that default to 4 for obs_decam (except not in the case of CP calibs, for the reasons described above). So DM-23616 effectively enabled an earlier override that had been inadvertently disabled.
            Hide
            swinbank John Swinbank added a comment - - edited

            Per discussion on Slack, the conclusion is to leave wcsFitter.order set to 4 for the time being, but to experiment with reducing it to 2 when a Jointcal-derived distortion model for obs_decam becomes available (DM-24431).

            Show
            swinbank John Swinbank added a comment - - edited Per discussion on Slack , the conclusion is to leave wcsFitter.order set to 4 for the time being, but to experiment with reducing it to 2 when a Jointcal-derived distortion model for obs_decam becomes available ( DM-24431 ).
            Hide
            swinbank John Swinbank added a comment -

            Eric — I've confirmed with local testing that this is the source of the change in metric value. Assuming you're happy with that, the only change on this ticket is to update the comments in obs_decam to explain the decision that I think has been reached on Slack for our future reference.

            PR: https://github.com/lsst/obs_decam/pull/138

            Show
            swinbank John Swinbank added a comment - Eric — I've confirmed with local testing that this is the source of the change in metric value. Assuming you're happy with that, the only change on this ticket is to update the comments in obs_decam to explain the decision that I think has been reached on Slack for our future reference. PR: https://github.com/lsst/obs_decam/pull/138
            Hide
            ebellm Eric Bellm added a comment -

            Looks good.

            Show
            ebellm Eric Bellm added a comment - Looks good.
            Hide
            swinbank John Swinbank added a comment -

            Thanks; merged.

            Show
            swinbank John Swinbank added a comment - Thanks; merged.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Eric Bellm
                Watchers:
                Eric Bellm, John Parejko, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel