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

Refactor PhotoCalTask


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


      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.


          Issue Links



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


                • Created:
                  Planned End:

                  Summary Panel