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

Switch the default matcher to PessimisticB.

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      AP F18-1, AP F18-2, AP F18-3, AP F18-5, AP F18-6, AP S19-1, AP S19-2
    • Team:
      Alert Production

      Description

      Thereby implementing RFC-491.

      Also implement the speedup discussed on that RFC.

        Attachments

          Issue Links

            Activity

            Show
            cmorrison Chris Morrison [X] (Inactive) added a comment - Jenkins passes:  https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29224/pipeline
            Hide
            swinbank John Swinbank added a comment -

            Chris Morrison [X] — I believe that the work to be reviewed here is on tickets/DM-14857 in obs_subaru, obs_cfht and meas_astrom; that the branches in jointcal and pipe_tasks are spurious; and that there is no work in other packages. Is that right?

            Show
            swinbank John Swinbank added a comment - Chris Morrison [X] — I believe that the work to be reviewed here is on tickets/ DM-14857 in obs_subaru, obs_cfht and meas_astrom; that the branches in jointcal and pipe_tasks are spurious; and that there is no work in other packages. Is that right?
            Hide
            swinbank John Swinbank added a comment - - edited

            Could you please explain what's going on in https://github.com/lsst/meas_astrom/commit/e8e2fa31ea7dd2a9b702a55d9e11a55171ba9a9a? In particular,

            • Why is this not just a simple swap of MatchPessimisticBTask for MatchOptimisticBTask?
            • You import DirectMatchTask into ref_match.py, but then set the target to directMatch, which looks wrong (indeed, undefined).
            • from .matchPessimisticB import matchPessimisticBTask in astrometry.py seems to have the wrong case (should be MatchPessimisticBTask). But in that case, this should be throwing an ImportError when the code is run... so how can it be passing Jenkins? I'm confused about what's going on. Can you enlighten me?
            Show
            swinbank John Swinbank added a comment - - edited Could you please explain what's going on in https://github.com/lsst/meas_astrom/commit/e8e2fa31ea7dd2a9b702a55d9e11a55171ba9a9a? In particular, Why is this not just a simple swap of MatchPessimisticBTask for MatchOptimisticBTask ? You import DirectMatchTask into ref_match.py , but then set the target to directMatch , which looks wrong (indeed, undefined). from .matchPessimisticB import matchPessimisticBTask in astrometry.py seems to have the wrong case (should be MatchPessimisticBTask ). But in that case, this should be throwing an ImportError when the code is run... so how can it be passing Jenkins? I'm confused about what's going on. Can you enlighten me?
            Hide
            swinbank John Swinbank added a comment -

            Oh, wait, ignore me — it looks as though that was all changed on later commits. (But not squashed? Hmm.)

            Show
            swinbank John Swinbank added a comment - Oh, wait, ignore me — it looks as though that was all changed on later commits. (But not squashed? Hmm.)
            Hide
            cmorrison Chris Morrison [X] (Inactive) added a comment -

            Sorry I should have stated this in the review before sending it to you.

             

            So those branches currently exist because Jenkins remembered previous ticket branches after I deleted them. I thought I initially needed them as they have settings for the old matcher in the repositories that come with them, but I turns out I do not need them. Jenkins remembered these really old branches and tried to run off them causing crashes. I then recreated the branches to basically have a copy of master that Jenkins would pull. 

            Show
            cmorrison Chris Morrison [X] (Inactive) added a comment - Sorry I should have stated this in the review before sending it to you.   So those branches currently exist because Jenkins remembered previous ticket branches after I deleted them. I thought I initially needed them as they have settings for the old matcher in the repositories that come with them, but I turns out I do not need them. Jenkins remembered these really old branches and tried to run off them causing crashes. I then recreated the branches to basically have a copy of master that Jenkins would pull. 
            Hide
            swinbank John Swinbank added a comment -

            Some mostly-fairly-minor comments on the code in meas_astrom at https://github.com/lsst/meas_astrom/pull/106. In addition to those, it would be great to clean up the history by squashing — per my confusion above, the unsquashed history makes it hard to understand what's going on.

            Aside from that, the changes in obs_cfht and obs_subaru look fine.

            Two naive questions:

            • Why aren't changes necessary in other packages? It looks as though Jointcal, pipe_tasks and a few other packages are referring to OptimisticB only config parameters — why don't they need updating? Am I missing something obvious?
            • I know you've demonstrated that PPMb performs well in earlier tests, but do we know exactly what impact merging this will have on the existing metrics that are being tracked in SQuaSH?
            Show
            swinbank John Swinbank added a comment - Some mostly-fairly-minor comments on the code in meas_astrom at https://github.com/lsst/meas_astrom/pull/106 . In addition to those, it would be great to clean up the history by squashing — per my confusion above, the unsquashed history makes it hard to understand what's going on. Aside from that, the changes in obs_cfht and obs_subaru look fine. Two naive questions: Why aren't changes necessary in other packages? It looks as though Jointcal, pipe_tasks and a few other packages are referring to OptimisticB only config parameters — why don't they need updating? Am I missing something obvious? I know you've demonstrated that PPMb performs well in earlier tests, but do we know exactly what impact merging this will have on the existing metrics that are being tracked in SQuaSH?
            Hide
            swinbank John Swinbank added a comment -

            Yusra AlSayyad, Jim Bosch — do you have any concerns merging this with regards to HSC processing?

            Show
            swinbank John Swinbank added a comment - Yusra AlSayyad , Jim Bosch — do you have any concerns merging this with regards to HSC processing?
            Hide
            jbosch Jim Bosch added a comment -

            No concerns from me. We'll need to remember to mention it to NAOJ - I'm sure they'll want to do some validation of their own before using it in an official HSC DR, but even if something goes wrong there we can just disable it via config.

            Show
            jbosch Jim Bosch added a comment - No concerns from me. We'll need to remember to mention it to NAOJ - I'm sure they'll want to do some validation of their own before using it in an official HSC DR, but even if something goes wrong there we can just disable it via config.
            Show
            swinbank John Swinbank added a comment - Good to go, modulo a tiny typo ( https://github.com/lsst/meas_astrom/pull/106/commits/09c3b8f1a0d28a28a8dc83fff2ac8e4c346ca930#r249313748 )
            Show
            cmorrison Chris Morrison [X] (Inactive) added a comment - Final successful Jenkis run check:  https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29300/pipeline/46

              People

              Assignee:
              cmorrison Chris Morrison [X] (Inactive)
              Reporter:
              swinbank John Swinbank
              Reviewers:
              John Swinbank
              Watchers:
              Chris Morrison [X] (Inactive), Jim Bosch, John Swinbank
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.