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

add base class for measurement tasks

    XMLWordPrintable

    Details

      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.

        Attachments

          Activity

          Hide
          jbosch 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.

          Show
          jbosch 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
          jbosch 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
          jbosch 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
          krughoff 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.

          Show
          krughoff 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
          jbosch 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
          jbosch 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.
          Hide
          jbosch 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
          jbosch 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:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Simon Krughoff
            Watchers:
            Jim Bosch, Simon Krughoff
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.