Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-589

Move AstrometryTask source selection from "matcher" into AstrometryTask

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      Currently in the stack (as of w_2019_12) the AstrometryTask in meas_astrom has two configurations for a SourceSelector. The first is config.astrometry.sourceSelection (a configurable field for the subtask), and the second is config.astrometry.matcher.sourceSelector (a sourceSelectorRegistry). It turns out that this first one is used when the AstrometryTask is called with config.astrometry.forceKnownWcs=True (as in characterizeImage.py) and the second one is used when the full WCS fit is performed (in calibrate.py).
      This is, at least, confusing (and certainly not documented), and possibly harmful when a user sets the wrong SourceSelector field.

      In addition, AstrometryTask has a ReferenceSelector that is only used when config.astrometry.forceKnownWcs=True and is not used at all when fitting a full WCS.  This incorrect behavior is addressed in DM-18643. However, working on that ticket has led me down a path to propose this RFC.

      Namely, I propose the following:

      • The sourceSelector be removed from the matcher tasks (namely, MatchOptimisticBTask and MatchPessimisticBTask)
      • The sourceSelection in AstrometryTask be changed to a SourceSelectorRegistry.
      • The source selection and reference selection (fixing DM-18643) will be called by AstrometryTask prior to the match-and-fit iterations. This will also make things (marginally) faster because in the current usage the (identical) source selection is repeated for each iteration.

      This has the advantage that (a) users expect source and reference selectors to be together; (b) These selectors can go either in the matcher or AstrometryTask; (c) currently there's a source selector in matcher and a fake source and reference selector masquerading in the AstrometryTask; (d) I believe the proposed relocation to AstrometryTask makes more sense and the author of the matcher (Chris Morrison [X]) approves.

      This will furthermore necessitate changing all the processCcd.py default configs in all the obs packages, as they all (to first order) make changes to the astrometry source selection settings.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Show
            jbosch Jim Bosch added a comment -
            Hide
            cmorrison Chris Morrison [X] (Inactive) added a comment -

            Sounds fine to me. I would point out that the current structure of a sourceSelector in both RefMatch task and the matcher is uses introduces some likely unexpected behavior where two different sourceSelectors are run over the same data (once in RefMatch and once in the matcher).

            Show
            cmorrison Chris Morrison [X] (Inactive) added a comment - Sounds fine to me. I would point out that the current structure of a sourceSelector in both RefMatch task and the matcher is uses introduces some likely unexpected behavior where two different sourceSelectors are run over the same data (once in RefMatch and once in the matcher).
            Show
            erykoff Eli Rykoff added a comment - Ah you mentioned this on slack and I only just noticed the code path that makes this happen: https://github.com/lsst/meas_astrom/blob/b71a5648e2899270502e19d7bb47e8521766682d/python/lsst/meas/astrom/ref_match.py#L125 https://github.com/lsst/meas_astrom/blob/b71a5648e2899270502e19d7bb47e8521766682d/python/lsst/meas/astrom/ref_match.py#L143 That seems less than ideal, yes.
            Hide
            erykoff Eli Rykoff added a comment -

            Adopting as-is since there were no complaints. I've changed DM-18643 into the triggered ticket.

            Show
            erykoff Eli Rykoff added a comment - Adopting as-is since there were no complaints. I've changed DM-18643 into the triggered ticket.

              People

              Assignee:
              erykoff Eli Rykoff
              Reporter:
              erykoff Eli Rykoff
              Watchers:
              Chris Morrison [X] (Inactive), Eli Rykoff, James Chiang, Jim Bosch, John Parejko, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins

                  No builds found.