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

Change star selectors to return stars instead of PSF candidates

    XMLWordPrintable

    Details

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

      Description

      Our current star selectors return a list of PsfCandidate s instead of source records. This makes them clumsy for many operations.

      I propose the following changes:

      • Add a base class for star selectors. Have this base class provide a method that turns a source catalog into a list of PsfCandidate. The default implementation will probably suffice in all cases, but if not, this offers natural way to override it.
      • Change the selectStars method to return an lsst.pipe.base.Struct that contains a catalog of sources that are believed to be stars.
      • Make star selectors into tasks. They are already configurable objects. This has several advantages, including adding a log object and eliminating the need for a registry. I suggest that the primary method retain the name selectStars, but if people prefer to rename it to run I don't mind.
      • Eliminate the star selector registry, unless it is still wanted.

      Note that there are alternatives to returning a source catalog that might do just as well, though I think a catalog is more natural:

      • Have the task flag stars. This is somewhat clumsy because it requires that the star selector add a field (whose name will presumably be a config parameter) and users must then scan the catalog to find stars. It does have the advantage of saving space, and users can easily find stars or non-stars equally easily. Which brings up a point discussed below.
      • Return a list of source IDs. This is somewhat clumsy for users but saves space.

      Will users ever want an easy want to identify non stars? Neither the current interface nor my proposed changes support. Obvious options include:

      • Set a flag in the catalog instead of returning a catalog, as mentioned above.
      • Return two two catalogs: stars and non-stars.
      • Have a flag that controls whether the output is stars or non-stars.

      Note that lsst.ip.diffim.DiaCatalogSourceSelector already returns a list of sources (an ordinary python list) rather than a list of PsfCandidate. It should be made the same as the others, and probably renamed from ...SourceSelector to ...StarSelector.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment - - edited

            Implementing the code in DM-5532 brought up a few issues that I would like feedback on.

            Some stars may be rejected when making PSF candidates (is this usually due to inadequate selection, or inherent?). Thus by not always making PSF candidates we lose some suitability information. If this is important, it can be ameliorated by making run create the candidate list, which could down-select the star catalog and flag only the final stars.

            At the other extreme, if we don't care about the fact that some stars cannot be made into good PSF candidates when selecting stars, then makePsfCandidates does not seem to be a natural method of star selectors. It uses two config parameters that are not needed by any star selector (except the second moment star selector, which also makes PSF candidates while selecting stars). To me it makes more sense for this to be a method of PSF determiners (e.g. implemented in a base class, and have the main method accept a star catalog), but that would expand the scope of this RFC. It could also be added to MeasurePsfTask or become standalone code. If it is added to PSF determiners then I'd be tempted to give them the same treatment as star selectors: a base class that is a task.

            Every star selector seems to have its own set of bad flags used to reject sources as unsuitable. Why the disparity? Can we reduce or eliminate it (which will change behavior)? This could be done in DM-5532 or on another ticket.

            Note that reneged on keeping the star selector registry because we have no nice way to make subtasks from a registry; the constructor is called with too little information. In the long term we may want to implement subtask registries, since it would let us group related subtasks.

            I think making PSF candidates should be simpler, so I filed DM-5578. That is not meant to be part of this RFC, I'm just noting it.

            Show
            rowen Russell Owen added a comment - - edited Implementing the code in DM-5532 brought up a few issues that I would like feedback on. Some stars may be rejected when making PSF candidates (is this usually due to inadequate selection, or inherent?). Thus by not always making PSF candidates we lose some suitability information. If this is important, it can be ameliorated by making run create the candidate list, which could down-select the star catalog and flag only the final stars. At the other extreme, if we don't care about the fact that some stars cannot be made into good PSF candidates when selecting stars, then makePsfCandidates does not seem to be a natural method of star selectors. It uses two config parameters that are not needed by any star selector (except the second moment star selector, which also makes PSF candidates while selecting stars). To me it makes more sense for this to be a method of PSF determiners (e.g. implemented in a base class, and have the main method accept a star catalog), but that would expand the scope of this RFC. It could also be added to MeasurePsfTask or become standalone code. If it is added to PSF determiners then I'd be tempted to give them the same treatment as star selectors: a base class that is a task. Every star selector seems to have its own set of bad flags used to reject sources as unsuitable. Why the disparity? Can we reduce or eliminate it (which will change behavior)? This could be done in DM-5532 or on another ticket. Note that reneged on keeping the star selector registry because we have no nice way to make subtasks from a registry; the constructor is called with too little information. In the long term we may want to implement subtask registries, since it would let us group related subtasks. I think making PSF candidates should be simpler, so I filed DM-5578 . That is not meant to be part of this RFC, I'm just noting it.
            Hide
            jbosch Jim Bosch added a comment -

            Making StarSelectors use a consistent set of flags by default sounds like a good idea (as long as the set of flags remains configurable), and I'd definitely endorse making the way those flags are configured consistent.. I suspect just adopting the flags used by ObjectSizeStarSelector is probably what we want. Should probably be a separate issue, though.

            I'm much more concerned about this extra selection done by PsfCandidate - we need to understand exactly what PsfCandidate is doing, and probably move that logic to Python if we want to move forward with this issue. Having additional (opaque) selection criteria imposed as side-effect to PsfCandidate construction seems like a design flaw that we'd like to fix, but we can't just drop the behavior and hope it isn't important.

            Show
            jbosch Jim Bosch added a comment - Making StarSelectors use a consistent set of flags by default sounds like a good idea (as long as the set of flags remains configurable), and I'd definitely endorse making the way those flags are configured consistent.. I suspect just adopting the flags used by ObjectSizeStarSelector is probably what we want. Should probably be a separate issue, though. I'm much more concerned about this extra selection done by PsfCandidate - we need to understand exactly what PsfCandidate is doing, and probably move that logic to Python if we want to move forward with this issue. Having additional (opaque) selection criteria imposed as side-effect to PsfCandidate construction seems like a design flaw that we'd like to fix, but we can't just drop the behavior and hope it isn't important.
            Hide
            rowen Russell Owen added a comment - - edited

            If we have the run method make PSF candidates then we can easily obtain the old behavior and return both a star catalog and an associated set of PSF candidates (both the same length in the same order), thus exactly preserving the old behavior even for users that only want stars (of which we don't yet have any – I had a use case, but it turned out to be unnecessary). This will remove extensive code duplication (the part that makes PSF candidates) and better isolate algorithm-specific code in one method (selectStars or, if we want to avoid having users accidentally select stars that cannot be made into PSF candidates, make it a private method).

            In any case, I agree it would be useful to understand why stars are being selected that cannot be made into PSF candidates. I'm trying to find some cases. I also agree that unifying flags may be best left for a separate, cleanup ticket. It has the potential to change behavior, so will require plenty of testing. It may be or may not be related to causes of failure for making PSF candidates.

            Show
            rowen Russell Owen added a comment - - edited If we have the run method make PSF candidates then we can easily obtain the old behavior and return both a star catalog and an associated set of PSF candidates (both the same length in the same order), thus exactly preserving the old behavior even for users that only want stars (of which we don't yet have any – I had a use case, but it turned out to be unnecessary). This will remove extensive code duplication (the part that makes PSF candidates) and better isolate algorithm-specific code in one method ( selectStars or, if we want to avoid having users accidentally select stars that cannot be made into PSF candidates, make it a private method). In any case, I agree it would be useful to understand why stars are being selected that cannot be made into PSF candidates. I'm trying to find some cases. I also agree that unifying flags may be best left for a separate, cleanup ticket. It has the potential to change behavior, so will require plenty of testing. It may be or may not be related to causes of failure for making PSF candidates.
            Hide
            rowen Russell Owen added a comment -

            No luck, so far, triggering failure of stars to be made into PSF candidates. None with the quick test for CFHT or DECam in validate_drp. I'll try the longer tests, but I think this is not worth holding up the RFC and code over.

            I do think it is important to report such errors more vocally than some of the star selectors were doing. The object size star selector (at least) was logging at debug level. I would rather report these as warnings (and if we get a flood of such message it would be easy to consolidate them down to a single report, or one per type of failure).

            So I propose the following:

            • Make StarSelector an abstract base task with three methods: selectStars, makePsfCandidates and run. The only abstract method is selectStars
            • Have run select stars and make a PSF candidate list and only return and flag stars that can be made into PSF candidates. This makes it very easy to use for PSF fitting (which is our current dominant use case), and assures that the list of PSF candidates and catalog of stars will match. At the same time you can still call selectStars to only get a star catalog, and subclasses need only override that one method.

            I have implemented that in DM-5532 and it seems to work nicely. Further refinements may be possible, but it is much nicer than the old code in that:

            • It is easy to get a star catalog
            • It reduces code duplication
            Show
            rowen Russell Owen added a comment - No luck, so far, triggering failure of stars to be made into PSF candidates. None with the quick test for CFHT or DECam in validate_drp. I'll try the longer tests, but I think this is not worth holding up the RFC and code over. I do think it is important to report such errors more vocally than some of the star selectors were doing. The object size star selector (at least) was logging at debug level. I would rather report these as warnings (and if we get a flood of such message it would be easy to consolidate them down to a single report, or one per type of failure). So I propose the following: Make StarSelector an abstract base task with three methods: selectStars , makePsfCandidates and run . The only abstract method is selectStars Have run select stars and make a PSF candidate list and only return and flag stars that can be made into PSF candidates. This makes it very easy to use for PSF fitting (which is our current dominant use case), and assures that the list of PSF candidates and catalog of stars will match. At the same time you can still call selectStars to only get a star catalog, and subclasses need only override that one method. I have implemented that in DM-5532 and it seems to work nicely. Further refinements may be possible, but it is much nicer than the old code in that: It is easy to get a star catalog It reduces code duplication
            Hide
            rowen Russell Owen added a comment -

            Adopted as follows:

            • Make star selectors tasks
            • Eliminate the registry, for now, in lieu of a registry field for subtasks (one that passes arguments such as log to the constructor)
            • Add an abstract base class for star selectors with the following methods:
            • selectStars abstract method that takes a catalog of sources and returns an lsst.pipe.base.Struct containing a catalog of stars
            • makePsfCandidates a concrete method that takes a catalog of stars (as returned by selectStars and produces PSF candidates; it logs a warning for any star that cannot be turned into a PSF candidate, and so returns an lsst.pipe.base.Struct a list of PSF candidates and (in the same order) a catalog of stars that were successfully made into PSF candidates
            • run a concrete method that takes a catalog of sources and an optional name of a flag field and calls selectStars, then makePsfCandidates, then optionally sets the flags of the final star catalog
            Show
            rowen Russell Owen added a comment - Adopted as follows: Make star selectors tasks Eliminate the registry, for now, in lieu of a registry field for subtasks (one that passes arguments such as log to the constructor) Add an abstract base class for star selectors with the following methods: selectStars abstract method that takes a catalog of sources and returns an lsst.pipe.base.Struct containing a catalog of stars makePsfCandidates a concrete method that takes a catalog of stars (as returned by selectStars and produces PSF candidates; it logs a warning for any star that cannot be turned into a PSF candidate, and so returns an lsst.pipe.base.Struct a list of PSF candidates and (in the same order) a catalog of stars that were successfully made into PSF candidates run a concrete method that takes a catalog of sources and an optional name of a flag field and calls selectStars , then makePsfCandidates , then optionally sets the flags of the final star catalog

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Watchers:
              Colin Slater, Jim Bosch, Kian-Tat Lim, Michael Wood-Vasey, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End: