Fix Version/s: None
Sprint:AP F18-1, AP F18-2, AP F18-3, AP F18-5, AP F18-6, AP S19-1, AP S19-2
- is blocked by
DM-14868 Adapt validation_data_* processing to use HTM catalogs
DM-15048 Tweak MatchPessimisticB configuration to ensure all unit tests pass
- is triggered by
RFC-491 Make MatchPessimisticB the default astrometry matcher in place of MatchOptimisticB
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?
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?
Oh, wait, ignore me — it looks as though that was all changed on later commits. (But not squashed? Hmm.)
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.
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?
Yusra AlSayyad, Jim Bosch — do you have any concerns merging this with regards to HSC processing?
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.
Good to go, modulo a tiny typo (https://github.com/lsst/meas_astrom/pull/106/commits/09c3b8f1a0d28a28a8dc83fff2ac8e4c346ca930#r249313748)
Final successful Jenkis run check: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29300/pipeline/46
Jenkins passes: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/29224/pipeline