Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: pipe_tasks
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:Measurement-S14-4
-
Team:Data Release Production
Description
CalibrateTask does some unexpected things differently if you configure it certain ways, because it perceives certain processing as only being necessary to feed other steps. In particular, if you disable astrometry and photometric calibration, it only runs measurement once, because it assumes the only purpose of the post-PSF measurement is to feed those algorithms. This (as well as poor test coverage) made it easy to break CalibrateTask in the case where those options are disabled a few branches back.
After conferring with Simon and Andy, we think the best solution is to remove this sort of conditional processing from CalibrateTask, which should also make it much easier to read. Instead, we'll always do both the initial and final phase of measurement, even if one of those phases is not explicitly being used within CalibrateTask itself.
Attachments
Activity
Perry, I'll get right on this.
Review comments:
python/lsst/obs/sdss/calibrate.py
line 315: It makes me nervous that the cause of this issue is conditionally running measurement and there is still a conditional on running measurement. Should I worry?
Other than that, looks fine to me.
I was attempting to make this change with minimal side-effects, so I left the code as I found except for some rearranging of conditionals. There is one call to measurement.measure in the not doPsf case, and a second call to measurement.run in the doPsf case. They are the same, so measurement gets called in either case.
Perry Gee
added a comment - I was attempting to make this change with minimal side-effects, so I left the code as I found except for some rearranging of conditionals. There is one call to measurement.measure in the not doPsf case, and a second call to measurement.run in the doPsf case. They are the same, so measurement gets called in either case. I see now. Thanks for the explanation. The doPsf case didn't show up in the diff. Looks fine to me.
Simon – could you review this, since I understand that the request came from you? Note that the final decision was to leave things alone, except to run both measurement steps regardless of other options. If that isn't what you had in mind, please send it back.
Changes are in two packages:
, 13 deletions
python/lsst/pipe/tasks/calibrate.py | 26 +++++++++++++-------------
1 file changed, 13 insertions
python/lsst/obs/sdss/calibrate.py | 14 +++++++-------
, 7 deletions
1 file changed, 7 insertions