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

Refactor PhotoCalTask

    Details

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

      Description

      In the course of adding selection of sources by color to photocal (DM-12207), I'd like to refactor PhotoCalTask, which has some problems:

      • It's not very modular: the selection of sources uses custom code and it's all thrown into a single long method.
      • It inherits from RefMatchTask, which seems a strange choice: the relationship with matching is more clearly a "has a" than an "is a".
      • It does a full astrometric match and fit rather than using the known astrometric solution (this is a common failure point, if only because the user only tweaks the main astrometry matcher and forgets the one in photocal).
      • The reservation of sources from the fitting matches functionality in MeasurePsfTask but has an independent implementation.

      To address these (and make it easier for me to implement and test the color selection) I propose:

      • Make the source selection more modular by using a SourceSelectorTask.
      • Have PhotoCalTask compose a matcher instead of inherit.
      • Use the DirectMatchTask to do the matching.
      • Factor the reservation of sources into a ReserveSourcesTask (and use this in MeasurePsfTask too).

      These changes will affect the configuration tree, which means that old configurations will not load cleanly (necessitating the use of --clobber-config when using new code with old data repos), and any applicable overrides will need to be revised. I can put together a mapping between the new and old values if desired; some should be obvious (e.g., reserveFraction --> reserve.fraction), while others a little more obscure (e.g., goodFlags --> match.sourceSelection.flags.good).

      A proposed implementation is available on DM-12207 if anyone wants to look at some concrete changes.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            +1 This sounds like a great idea.

            Show
            Parejkoj John Parejko added a comment - +1 This sounds like a great idea.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Sounds great in general, but are there any changes to the actual logic or output fields involved in source reservation for various tasks?

            Show
            jbosch Jim Bosch added a comment - - edited Sounds great in general, but are there any changes to the actual logic or output fields involved in source reservation for various tasks?
            Hide
            price Paul Price added a comment - - edited

            Thanks Jim, I forgot to mention that. The way I have it now the following fields are changed:

            • calib_photometryUsed --> calib_photometry_used
            • calib_photometryCandidate removed
            • calib_photometryReserved --> calib_photometry_reserved
            • calib_psfReserved --> calib_psf_reserved
            Show
            price Paul Price added a comment - - edited Thanks Jim, I forgot to mention that. The way I have it now the following fields are changed: calib_photometryUsed --> calib_photometry_used calib_photometryCandidate removed calib_photometryReserved --> calib_photometry_reserved calib_psfReserved --> calib_psf_reserved
            Hide
            price Paul Price added a comment -

            Adopted. Implementation has passed Jenkins and is in review.

            Show
            price Paul Price added a comment - Adopted. Implementation has passed Jenkins and is in review.
            Hide
            tjenness Tim Jenness added a comment -

            Paul Price I see the triggered work ticket has been closed. Can this RFC now be marked as implemented?

            Show
            tjenness Tim Jenness added a comment - Paul Price I see the triggered work ticket has been closed. Can this RFC now be marked as implemented?

              People

              • Assignee:
                price Paul Price
                Reporter:
                price Paul Price
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Paul Price, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel