XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
1
• Sprint:
Measurement-S14-3
• Team:
Data Release Production

#### Description

We should consider adding a base class for measurement tasks (SingleFrameMeasurementTask, ForcedMeasuremedTask) that includes the callMeasure methods. I'm hoping this will help cleanup callMeasure and improve code reuse.

#### Activity

Hide
Jim Bosch added a comment -

Ready for review. Looks like a lot of changes, but it's just shuffling code around, so it's actually pretty simple:

• moved ForcedMeasurementTask and its helper classes out of forcedImage.py into forcedMeasurement.py;
• moved NoiseReplacer out of base.py into noiseReplacer.py;

In addition, improving the docs for all of these classes is DM-976, so please don't worry about deficiencies there for now.

Show
Jim Bosch added a comment - Ready for review. Looks like a lot of changes, but it's just shuffling code around, so it's actually pretty simple: moved ForcedMeasurementTask and its helper classes out of forcedImage.py into forcedMeasurement.py; moved NoiseReplacer out of base.py into noiseReplacer.py; added a new base class, BaseMeasurementTask, for SingleFrameMeasurementTask and ForcedMeasurementTask, which now includes the callMeasure and callMeasureN functions as methods. In addition, improving the docs for all of these classes is DM-976 , so please don't worry about deficiencies there for now.
Hide
Jim Bosch added a comment -

Oops, forgot to mention that all changes are on u/jbosch/DM-978 of meas_base:

 meas_base:u/jbosch/DM-978 % git diff --stat master...u/jbosch/DM-978  python/lsst/meas/base/__init__.py | 2 +  python/lsst/meas/base/base.py | 473 ++++------------------------  python/lsst/meas/base/forcedImage.py | 371 +---------------------  python/lsst/meas/base/forcedMeasurement.py | 361 +++++++++++++++++++++  python/lsst/meas/base/noiseReplacer.py | 388 +++++++++++++++++++++++  python/lsst/meas/base/plugins.py | 2 +-  python/lsst/meas/base/sfm.py | 22 +-  tests/testForced.py | 13 +-  tests/testSingleFrameMeasurement.py | 3 +-  9 files changed, 852 insertions(+), 783 deletions(-)

Show
Jim Bosch added a comment - Oops, forgot to mention that all changes are on u/jbosch/ DM-978 of meas_base: meas_base:u/jbosch/DM-978 % git diff --stat master...u/jbosch/DM-978 python/lsst/meas/base/__init__.py | 2 + python/lsst/meas/base/base.py | 473 ++++------------------------ python/lsst/meas/base/forcedImage.py | 371 +--------------------- python/lsst/meas/base/forcedMeasurement.py | 361 +++++++++++++++++++++ python/lsst/meas/base/noiseReplacer.py | 388 +++++++++++++++++++++++ python/lsst/meas/base/plugins.py | 2 +- python/lsst/meas/base/sfm.py | 22 +- tests/testForced.py | 13 +- tests/testSingleFrameMeasurement.py | 3 +- 9 files changed, 852 insertions(+), 783 deletions(-)
Hide
Simon Krughoff added a comment -

I know this code is mostly reorg, but it's hard to tell if the reorg broke anything because the tasks that were touched don't have unit test coverage.

1. forcedImage and forcedCcd are not in _init_.py. Should they be?
2. I know it's hard to test tasks, but processForcedImageTask doesn't appear to be in any unit tests.
3. In forcedImage.py: I don't know how the run method in ProcessImageForcedTask can work since the subtask name is measurement, but there is no measurement entry in the config. Also, self.forcedMeasurement is referenced in run(), but I don't think it exists. See #2.
4. MeasurementDataFlags used on line 63 of forcedImage.py, but is not imported.
5. In sfm.py: In the constructor of SingleFrameMeasurementTask I think we have agreed that super is preferable to calling the _init_ method on the base class, but I could be wrong.
6. sfm.py line 242: I don't think NoiseReplacer was imported.
7. in testForced.py: ProcessForcedCcdTask is imported, but never used.

Show
Simon Krughoff added a comment - Here are my comments: I know this code is mostly reorg, but it's hard to tell if the reorg broke anything because the tasks that were touched don't have unit test coverage. 1. forcedImage and forcedCcd are not in _ init _.py. Should they be? 2. I know it's hard to test tasks, but processForcedImageTask doesn't appear to be in any unit tests. 3. In forcedImage.py: I don't know how the run method in ProcessImageForcedTask can work since the subtask name is measurement, but there is no measurement entry in the config. Also, self.forcedMeasurement is referenced in run(), but I don't think it exists. See #2. 4. MeasurementDataFlags used on line 63 of forcedImage.py, but is not imported. 5. In sfm.py: In the constructor of SingleFrameMeasurementTask I think we have agreed that super is preferable to calling the _ init _ method on the base class, but I could be wrong. 6. sfm.py line 242: I don't think NoiseReplacer was imported. 7. in testForced.py: ProcessForcedCcdTask is imported, but never used.
Hide
Jim Bosch added a comment -

1. forcedImage and forcedCcd are not in init.py. Should they be?

Might as well. I was thinking we didn't need to put things into __init__.py if they were only used internally by the package or imported by a particular bin script, but I suppose it also doesn't do any harm.

2. I know it's hard to test tasks, but processForcedImageTask doesn't appear to be in any unit tests.

Yeah, that was a known issue I'd planned to resolve later, but some of the other things you've brought up suggest that I really need to resolve it now, or at least do a one-off test. I might be able to add a test in pipe_tasks that would use some of the machinery I built for testing CoaddPsf, which would be better still, if still not quite ideal.

3. In forcedImage.py: I don't know how the run method in ProcessImageForcedTask can work since the subtask name is measurement, but there is no measurement entry in the config. Also, self.forcedMeasurement is referenced in run(), but I don't think it exists. See #2.

Definitely an oversight; subtask name switched from forcedMeasurement to measurement on this branch. Will fix.

4. MeasurementDataFlags used on line 63 of forcedImage.py, but is not imported.

Will fix.

5. In sfm.py: In the constructor of SingleFrameMeasurementTask I think we have agreed that super is preferable to calling the init method on the base class, but I could be wrong.

Yeah, that proposal is at least in the worst, and likely to pass if it hasn't already. Will change.

6. sfm.py line 242: I don't think NoiseReplacer was imported.

Will fix.

7. in testForced.py: ProcessForcedCcdTask is imported, but never used.

Not surprised. We need to do a lot of import pruning on the tests. I'll fix this one at least.

Show
Hide
Jim Bosch added a comment -

I believe I've addressed all of Simon's concerns, including adding some basic "does this run" test coverage for the forced photometry command-line tasks to pipe_tasks (some existing test infrastructure there made it much easier to test things there). Will merge to master as soon as I've verified via buildbot that I haven't broken the stack in any way.

Show
Jim Bosch added a comment - I believe I've addressed all of Simon's concerns, including adding some basic "does this run" test coverage for the forced photometry command-line tasks to pipe_tasks (some existing test infrastructure there made it much easier to test things there). Will merge to master as soon as I've verified via buildbot that I haven't broken the stack in any way.

#### People

Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Simon Krughoff
Watchers:
Jim Bosch, Simon Krughoff