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

Add "usesMatches" method to star selectors

    Details

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

      Description

      We have a standard API for star selectors, including a selectStars method that optionally takes matches, a list of source/reference object matches. Some star selectors require this argument, whereas others ignore it (and we may eventually have selectors that use it if provided), but there is no way to tell if matches are used by querying the star selector.

      In some cases the user may have to make an extra call to obtain matches, but there is no easy way to tell if this call would be useful. for instance a new DetectAndMeasure task runs a star selector if computing aperture corrections and it will do its own matching if required.

      I propose adding a new method usesMatches() to all star selectors. It returns True if selectStars can use the matches argument and False otherwise.

      Variants I considered, but am mildly against:

      • Adding a second requiresMatches() method. I don't think it is needed, though it would allow slightly earlier error reporting (e.g. if matches are required but no matcher is provided).
      • Making usesMatches a property instead of a method. Some star selectors are C++, though admittedly they need wrapping, so a property is possible; it's just more work.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            You may be right, but I think it is worth examining an example and exploring options.

            This version of CalibrateTask in pipe_tasks tickets/DM-4692 supports all star selectors (catalog and non). The user simply specifies the star selector as usual and it all just works. The part that is a bit complex is that an astrometry subtask is built if doing any of the following:

            • astrometric calibration (naturally)
            • a load-and-match function is required, which in this case is either due to performing photometric calibration (as now, since PhotoCalTask does have the ability to load and match) or if measuring aperture correction and using a star selector that can use a match list.

            Under the hood the associated DetectAndMeasureTask simply takes an optional loadAndMatch function that is used if appropriate. I have added a usesMatches method to that task, so the user can see when it might be useful.

            To my mind is offers useful functionality without too much pain. It would be a tad cleaner if the star selector returned a source catalog, but that's another RFC (one I intend to file, if this one goes through, but not work on until well after DM-4692).

            The important question is: how would this code be written if the catalog star selector was an entirely different sort of beast than the other star selectors? Is there a way to avoid rewriting this as two variant tasks, both with a fair amount of code duplication?

            I also wonder if we are likely to ever have star selectors that are primarily based upon something other than a match list, but can use a match list if available?

            Show
            rowen Russell Owen added a comment - - edited You may be right, but I think it is worth examining an example and exploring options. This version of CalibrateTask in pipe_tasks tickets/ DM-4692 supports all star selectors (catalog and non). The user simply specifies the star selector as usual and it all just works. The part that is a bit complex is that an astrometry subtask is built if doing any of the following: astrometric calibration (naturally) a load-and-match function is required, which in this case is either due to performing photometric calibration (as now, since PhotoCalTask does have the ability to load and match) or if measuring aperture correction and using a star selector that can use a match list. Under the hood the associated DetectAndMeasureTask simply takes an optional loadAndMatch function that is used if appropriate. I have added a usesMatches method to that task, so the user can see when it might be useful. To my mind is offers useful functionality without too much pain. It would be a tad cleaner if the star selector returned a source catalog, but that's another RFC (one I intend to file, if this one goes through, but not work on until well after DM-4692 ). The important question is: how would this code be written if the catalog star selector was an entirely different sort of beast than the other star selectors? Is there a way to avoid rewriting this as two variant tasks, both with a fair amount of code duplication? I also wonder if we are likely to ever have star selectors that are primarily based upon something other than a match list, but can use a match list if available?
            Hide
            rhl Robert Lupton added a comment -

            I agree with Russell; a catalog star selector is logically a star selector like any other and I'd like to see it pluggable using the same mechanism if possible.

            Show
            rhl Robert Lupton added a comment - I agree with Russell; a catalog star selector is logically a star selector like any other and I'd like to see it pluggable using the same mechanism if possible.
            Hide
            rowen Russell Owen added a comment -

            A minor variant to consider, if we do accept the basic idea: have usesStarSelector return an enum with values: NO=0, YES=1, REQUIRED=2. The return value could be treated as a boolean, but the details are available if needed. It's a minor change, but in the interest of providing a sensible namespace for the enum values, I suggest it be accompanied by adding a trivial base class for star selectors that has these enums. It could accept a constructor argument or be a metaclass so that the subclass just passes the desired value for usesStarSelector to the parent class.

            Show
            rowen Russell Owen added a comment - A minor variant to consider, if we do accept the basic idea: have usesStarSelector return an enum with values: NO=0, YES=1, REQUIRED=2. The return value could be treated as a boolean, but the details are available if needed. It's a minor change, but in the interest of providing a sensible namespace for the enum values, I suggest it be accompanied by adding a trivial base class for star selectors that has these enums. It could accept a constructor argument or be a metaclass so that the subclass just passes the desired value for usesStarSelector to the parent class.
            Hide
            rowen Russell Owen added a comment -

            In private communication Jim Bosch said he was willing to go along with it. If it is not wanted in the long term it can be changed as part of a larger overhaul of star selectors.

            He also pointed out that ShapeMagnitudeStarSelector is not maintained, not tested and and not used, so it has probably bit-rotted. It is the only C++ star selector at this time, and it's not worth updating it for this RFC. However, I think if we do have C++ star selectors in the future we should wrap them in a new small class that delegates selectStars (thus handling matches=None and allowing arguments by name) and that can implement this RFC.

            Show
            rowen Russell Owen added a comment - In private communication Jim Bosch said he was willing to go along with it. If it is not wanted in the long term it can be changed as part of a larger overhaul of star selectors. He also pointed out that ShapeMagnitudeStarSelector is not maintained, not tested and and not used, so it has probably bit-rotted. It is the only C++ star selector at this time, and it's not worth updating it for this RFC. However, I think if we do have C++ star selectors in the future we should wrap them in a new small class that delegates selectStars (thus handling matches=None and allowing arguments by name) and that can implement this RFC.
            Hide
            rowen Russell Owen added a comment - - edited

            Adopted with the following change: usersMatches will be a bool class variable instead of a method. It will be implemented as part of DM-4962

            If in the future we find a need to differentiate between "can use a star selector" and "requires a star selector" we can revisit using an enum that can be also used as a boolean for backwards compatibility.

            Show
            rowen Russell Owen added a comment - - edited Adopted with the following change: usersMatches will be a bool class variable instead of a method. It will be implemented as part of DM-4962 If in the future we find a need to differentiate between "can use a star selector" and "requires a star selector" we can revisit using an enum that can be also used as a boolean for backwards compatibility.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Watchers:
                David Nidever [X] (Inactive), Jim Bosch, John Swinbank, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel