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

Replace jointcal.StarSelector with meas_algorithms.starSelector

    Details

    • Story Points:
      2
    • Sprint:
      Alert Production X16 - 5, Alert Production F16 - 6, Alert Production F16 - 7
    • Team:
      Alert Production

      Description

      jointcal has its own custom star selector. This should be removed and replaced with a star selector based on meas_algorithms.starSelector. A good choice might be meas_algorithms.objectSizeStarSelector.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Started on this. Digging into the different starSelector options.

            Show
            Parejkoj John Parejko added a comment - Started on this. Digging into the different starSelector options.
            Hide
            Parejkoj John Parejko added a comment -

            Now that I've started digging, it turns out we'll certainly need a new "sourceSelector". Jointcal doesn't actually care so much whether things are stars or extended, it turns out, so it's not really a "starSelector": does this suggest BaseStarSelectorTask should really be BaseSourceSelectorTask?

            Critical things are non-blendedness (parent == 0 and nChildren == 0), an error limit (e.g. S/N > 10), which current selectors don't have.

            Show
            Parejkoj John Parejko added a comment - Now that I've started digging, it turns out we'll certainly need a new "sourceSelector". Jointcal doesn't actually care so much whether things are stars or extended, it turns out, so it's not really a "starSelector": does this suggest BaseStarSelectorTask should really be BaseSourceSelectorTask? Critical things are non-blendedness ( parent == 0 and nChildren == 0 ), an error limit (e.g. S/N > 10), which current selectors don't have.
            Hide
            rhl Robert Lupton added a comment -

            Jim and I have just discussed this with Pierre, and we agree it's a question of where we put the interface. Jointcal would be happy to accept a pre-filtered catalogue. Jointcal needs to be passed a star/galaxy flag so that it can handle proper motions correctly.

            We should take Pierre's "selector" as the initial one to use, although he says it isn't very good.

            Show
            rhl Robert Lupton added a comment - Jim and I have just discussed this with Pierre, and we agree it's a question of where we put the interface. Jointcal would be happy to accept a pre-filtered catalogue. Jointcal needs to be passed a star/galaxy flag so that it can handle proper motions correctly. We should take Pierre's "selector" as the initial one to use, although he says it isn't very good.
            Hide
            rowen Russell Owen added a comment -

            I agree that source selector might be a better name than star selector, and would not stand in the way of an RFC. However, I think focusing on the needed algorithm is more important. The code that is closest to what you want is probably in matchOptimisticB.py, since it selects sources based on what it thinks would be good candidates for fitting astrometry. Unfortunately that code is ported from HSC and does use a star selector. But turning it into a star selector would be a useful thing to do, and would probably be easy.

            Show
            rowen Russell Owen added a comment - I agree that source selector might be a better name than star selector, and would not stand in the way of an RFC. However, I think focusing on the needed algorithm is more important. The code that is closest to what you want is probably in matchOptimisticB.py , since it selects sources based on what it thinks would be good candidates for fitting astrometry. Unfortunately that code is ported from HSC and does use a star selector. But turning it into a star selector would be a useful thing to do, and would probably be easy.
            Hide
            sullivan Ian Sullivan added a comment -

            Looks good and fairly straightforward overall. I mostly had a few comments about clarity and consistency with other code. Very helpful comments throughout the code made understanding most functionality easy.

            Show
            sullivan Ian Sullivan added a comment - Looks good and fairly straightforward overall. I mostly had a few comments about clarity and consistency with other code. Very helpful comments throughout the code made understanding most functionality easy.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for the quick review. I've pushed updates against your comments. Jointcal still isn't happy with it, so I may need to tweak it more.

            Show
            Parejkoj John Parejko added a comment - Thanks for the quick review. I've pushed updates against your comments. Jointcal still isn't happy with it, so I may need to tweak it more.
            Hide
            Parejkoj John Parejko added a comment - - edited

            The jointcal tests are now happy with astrometrySourceSelector: jointcal runs to completion with 2 and 10 catalogs, though this sourceSelector picks more sources than the built-in one (astrometrySourceSelector doesn't have a check like the consistency of variances and second moments thing).

            However, the relative RMS for 10 catalogs has gone above my threshold slightly (that threshold was determined from a previous run of jointcal with its built-in selector). Not sure whether to raise the threshold or try to tighten astrometrySourceSelector's criteria.

            Show
            Parejkoj John Parejko added a comment - - edited The jointcal tests are now happy with astrometrySourceSelector: jointcal runs to completion with 2 and 10 catalogs, though this sourceSelector picks more sources than the built-in one (astrometrySourceSelector doesn't have a check like the consistency of variances and second moments thing). However, the relative RMS for 10 catalogs has gone above my threshold slightly (that threshold was determined from a previous run of jointcal with its built-in selector). Not sure whether to raise the threshold or try to tighten astrometrySourceSelector's criteria.
            Hide
            Parejkoj John Parejko added a comment -

            Ian: could you please take another look at this, now that I've added the vectorized code? It's a few added methods, no test changes.

            Show
            Parejkoj John Parejko added a comment - Ian: could you please take another look at this, now that I've added the vectorized code? It's a few added methods, no test changes.
            Hide
            Parejkoj John Parejko added a comment -

            Nate: please review the jointcal pull request (Ian's doing the meas_algorithms one).

            Show
            Parejkoj John Parejko added a comment - Nate: please review the jointcal pull request (Ian's doing the meas_algorithms one).
            Hide
            sullivan Ian Sullivan added a comment -

            The changes in meas_algorithms are straightforward, so that package is ready to go. If the code duplication caused by keeping both vectorized and non-vectorized versions of functions could be removed, that would be best, but that decision is beyond the scope of this ticket.

            Show
            sullivan Ian Sullivan added a comment - The changes in meas_algorithms are straightforward, so that package is ready to go. If the code duplication caused by keeping both vectorized and non-vectorized versions of functions could be removed, that would be best, but that decision is beyond the scope of this ticket.
            Hide
            rowen Russell Owen added a comment -

            I am unhappy about the code duplication and would rather require a contiguous catalog. I think we can get away with that because objectSize star selector requires it. However, I admit that is a change.

            Show
            rowen Russell Owen added a comment - I am unhappy about the code duplication and would rather require a contiguous catalog. I think we can get away with that because objectSize star selector requires it. However, I admit that is a change.
            Hide
            nlust Nate Lust added a comment -

            I too would like a resolution to the code duplication and contiguous catalog, but that is outside the scope of this work I think. I only have two minor comments on the ticket, otherwise the code looks fine.

            Show
            nlust Nate Lust added a comment - I too would like a resolution to the code duplication and contiguous catalog, but that is outside the scope of this work I think. I only have two minor comments on the ticket, otherwise the code looks fine.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for the reviews. jointcal branch now merged to master.

            I too am unhappy about the code duplication, but I'd much rather have it work now for either case and sort out a proper solution to the contiguity question.

            Show
            Parejkoj John Parejko added a comment - Thanks for the reviews. jointcal branch now merged to master. I too am unhappy about the code duplication, but I'd much rather have it work now for either case and sort out a proper solution to the contiguity question.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Ian Sullivan, Nate Lust
                Watchers:
                Ian Sullivan, John Parejko, Nate Lust, Robert Lupton, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel