Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9832

Cleanup and unify star selector call signatures

    Details

    • Epic Name:
      f18-ap-starselector
    • Story Points:
      40
    • WBS:
      02C.03.05
    • Team:
      Alert Production
    • Cycle:
      Fall 2018

      Description

      Implement RFC-198 and RFC-335 as described.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            In Progress notes:

            1. First review is DM-14233 (remove secondMoment), because everything else depends on not worrying about its API.
            2. Second review is DM-14102 (makePsfCandidates), because unifying the star/source selectors depends on getting this out of starSelector.run().
            3. Finally, bring the remaining starSelectors and sourceSelectors into compliance with the new base class. Merge these into this epic branch when their tests all pass.
            4. Rebase cleanup this epic branch (to squash out the lessons learned and API cleanups that were part of 3).
            5. Put this epic branch into review.

            Show
            Parejkoj John Parejko added a comment - In Progress notes: 1. First review is DM-14233 (remove secondMoment), because everything else depends on not worrying about its API. 2. Second review is DM-14102 (makePsfCandidates), because unifying the star/source selectors depends on getting this out of starSelector.run(). 3. Finally, bring the remaining starSelectors and sourceSelectors into compliance with the new base class. Merge these into this epic branch when their tests all pass. 4. Rebase cleanup this epic branch (to squash out the lessons learned and API cleanups that were part of 3). 5. Put this epic branch into review.
            Show
            Parejkoj John Parejko added a comment - - edited Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/27935/pipeline
            Hide
            Parejkoj John Parejko added a comment -

            Update on the plan above: in light of my desire to have this functionality in jointcal (e.g. DM-14155), we're going to review and merge the SourceSelector-only part of this epic first, and do the StarSelector part separately (besides Flagged, which was converted here).

            Show
            Parejkoj John Parejko added a comment - Update on the plan above: in light of my desire to have this functionality in jointcal (e.g. DM-14155 ), we're going to review and merge the SourceSelector-only part of this epic first, and do the StarSelector part separately (besides Flagged, which was converted here).
            Hide
            Parejkoj John Parejko added a comment -

            Jim Bosch: I hope you're still available to take on this moderate-sized review.

            This review is for the the (rebase squashed) combination of the above "Done" tickets (cleaning up the SourceSelector API, bringing the existing SourceSelectors inline with it, and converting FlaggedStarSelector to a SourceSelector), plus an implementation of RFC-335.

            The above Jenkins run didn't complete successfully due to a problem on lsst_ci master, but I believe I've caught everything in the existing PRs. I will ping here if there's any additional changes necessary.

            Show
            Parejkoj John Parejko added a comment - Jim Bosch : I hope you're still available to take on this moderate-sized review. This review is for the the (rebase squashed) combination of the above "Done" tickets (cleaning up the SourceSelector API, bringing the existing SourceSelectors inline with it, and converting FlaggedStarSelector to a SourceSelector), plus an implementation of RFC-335 . The above Jenkins run didn't complete successfully due to a problem on lsst_ci master, but I believe I've caught everything in the existing PRs. I will ping here if there's any additional changes necessary.
            Hide
            jbosch Jim Bosch added a comment -

             

            Review complete; most comments on the PRs.

            One more big-picture comment that doesn't appear there: I feel like the SourceSelector design overall conflates pluggability of particular algorithms for filtering with predefined combinations of filters that are useful for particular purposes; there are concrete SourceSelectors that fit into both categories.  I think it's a natural response to the way our configuration system and table library work, and doing better could be quite difficult (see below), but the result of this is that if you have a SourceSelector that applies N-1 cuts you care about, and another one that applies the Nth one, the only way to combine them is to write a new SourceSelector.  That has already lead to a proliferation of very similar SourceSelectors, and hence a lot of boilerplate.

            My ideal design for this would have a single nonpolymorphic SourceSelector Task with:

            • a registry of pluggable polymorphic predicate objects that each can compute a single non-trivial boolean for each source;
            • a DSL for combining those predicates with trivial less--than/greater-than/equals (etc) tests on fields using boolean logic; expressions in that DSL would be part of the configuration of the SourceSelectorTask;
            • a mechanism for assigning particular expressions (e.g. "what we use for PSF star selection") to particular flag fields, allowing them to be used both as outputs for later diagnostic use and as inputs to other expressions.

            That's obviously extremely ambitious, and I haven't thought through how we'd want to go about accomplishing it (or even whether it'd be worthwhile to try).  I'm mentioning it here largely so others can be on the lookout for low-hanging fruit that moves in something like that direction when we run into the limitations of the current design in the future.

            Show
            jbosch Jim Bosch added a comment -   Review complete; most comments on the PRs. One more big-picture comment that doesn't appear there: I feel like the SourceSelector design overall conflates pluggability of particular algorithms for filtering with predefined combinations of filters that are useful for particular purposes; there are concrete SourceSelectors that fit into both categories.  I think it's a natural response to the way our configuration system and table library work, and doing better could be quite difficult (see below), but the result of this is that if you have a SourceSelector that applies N-1 cuts you care about, and another one that applies the Nth one, the only way to combine them is to write a new SourceSelector.  That has already lead to a proliferation of very similar SourceSelectors, and hence a lot of boilerplate. My ideal design for this would have a single nonpolymorphic SourceSelector Task with: a registry of pluggable polymorphic predicate objects that each can compute a single non-trivial boolean for each source; a DSL for combining those predicates with trivial less--than/greater-than/equals (etc) tests on fields using boolean logic; expressions in that DSL would be part of the configuration of the SourceSelectorTask; a mechanism for assigning particular expressions (e.g. "what we use for PSF star selection") to particular flag fields, allowing them to be used both as outputs for later diagnostic use and as inputs to other expressions. That's obviously extremely ambitious, and I haven't thought through how we'd want to go about accomplishing it (or even whether it'd be worthwhile to try).  I'm mentioning it here largely so others can be on the lookout for low-hanging fruit that moves in something like that direction when we run into the limitations of the current design in the future.
            Hide
            Parejkoj John Parejko added a comment -

            I agree with your comment above about proliferation of similar SourceSelectors. I think Paul Price's generic selectors in sourceSelector.py get us some of the way toward dealing with this, by allowing one to build a sourceSelector from a handful of components. As I said in response to a similar comment on a PR: lets run with the current design for a while and see how it shakes out.

            Show
            Parejkoj John Parejko added a comment - I agree with your comment above about proliferation of similar SourceSelectors. I think Paul Price 's generic selectors in sourceSelector.py get us some of the way toward dealing with this, by allowing one to build a sourceSelector from a handful of components. As I said in response to a similar comment on a PR: lets run with the current design for a while and see how it shakes out.
            Hide
            Parejkoj John Parejko added a comment -

            There are three StarSelectors left that don't conform to the new API (see the open tickets above); One of them can probably be deleted. I suspect the best approach is to move them to another epic, along with the cleanup of the "interpolated in badFlags" ticket.

            RFC-198 is almost implemented!

            Show
            Parejkoj John Parejko added a comment - There are three StarSelectors left that don't conform to the new API (see the open tickets above); One of them can probably be deleted. I suspect the best approach is to move them to another epic, along with the cleanup of the "interpolated in badFlags" ticket. RFC-198 is almost implemented!

              People

              • Assignee:
                cmorrison Chris Morrison
                Reporter:
                cmorrison Chris Morrison
                Reviewers:
                Jim Bosch
                Watchers:
                Chris Morrison, Jim Bosch, John Parejko, John Swinbank
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel