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

refactor forced tasks into two tasks

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:

      Description

      After looking at it a bit more, I think we should refactor the current meas_base forced photometry task to separate the CmdLineTask from the Measurement task. This will allow the forced measurement task to share a common base class with SingleFrameMeasurementTask (allowing us to move the callPlugin free functions into that base class), and give us better parallels with existing tasks:

      • ProcesImageForcedTask (my proposed name for the base command-line task) will be more similar to ProcessImageTask. We'll also have ProcessForcedCcdTask and ProcessForcedCoaddTask.
      • ForcedMeasurementTask will be more similar to SingleFrameMeasurementTask.

      In short, I think this will both clean up the ugliness in callPlugin and make the whole hierarchy easier for newcomers to understand.

        Attachments

          Activity

          Hide
          pgee Perry Gee added a comment -

          Completed the refactoring and renaming of classes. Did not rename any of the python modules (forcedImage, forcedCcd, or forcedCoadd) even though I don't like any of them. We can change those now or later.

          On tickets/DM-241
          meas_base:

          bin/forcedCcd.py | 4 +-
          bin/forcedCoadd.py | 4 +-
          python/lsst/meas/base/forcedCcd.py | 18 ++--
          python/lsst/meas/base/forcedCoadd.py | 12 +--
          python/lsst/meas/base/forcedImage.py | 175 +++++++++++++++++++---------------
          tests/testForced.py | 34 +++----
          6 files changed, 130 insertions, 117 deletions

          obs_lsstSim:

          policy/LsstSimMapper.paf | 64 ++++++++++++++++++++++++++++++++++++++++++++++
          1 file changed, 64 insertions

          Please review these 8 new datasets. I'm not really sure they are in the right place.

          Show
          pgee Perry Gee added a comment - Completed the refactoring and renaming of classes. Did not rename any of the python modules (forcedImage, forcedCcd, or forcedCoadd) even though I don't like any of them. We can change those now or later. On tickets/ DM-241 meas_base: bin/forcedCcd.py | 4 +- bin/forcedCoadd.py | 4 +- python/lsst/meas/base/forcedCcd.py | 18 ++-- python/lsst/meas/base/forcedCoadd.py | 12 +-- python/lsst/meas/base/forcedImage.py | 175 +++++++++++++++++++--------------- tests/testForced.py | 34 +++---- 6 files changed, 130 insertions , 117 deletions obs_lsstSim: policy/LsstSimMapper.paf | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions Please review these 8 new datasets. I'm not really sure they are in the right place.
          Hide
          jbosch Jim Bosch added a comment -

          Review complete. A few minor comments, and one moderate one:

          • Docstring at forcedImage.py:242 about ForcedMeasurementTask needing to be subclassed for CCDs and Coadds is now obsolete.
          • Also in ForcedMeasurementTask: you should always use **kwds in a Task's __init__, and pass those to Task.__init__. Leave it to that to handle the config, parentTask, and name arguments. In the TestForcedMeasurementTask, I think you can just remove the __init__ method entirely (or maybe you could just remove the class?)
          • The "expregion" and "newsource" variables in generateSources() should be "expRegion" and "newSources", respectively, to comply with LSST code standards.
          • What do you think about moving generateSources() to ProcessImageForcedTask? ProcessImageForcedTask.run() could then call generateSources(), and then pass the SourceCatalog to be filled to ForcedMeasurementTask. This would make ForcedMeasurementTask a little more similar to SingleFrameMeasurementTask, and then generateSources() could then call makeIdFactory() itself.
          Show
          jbosch Jim Bosch added a comment - Review complete. A few minor comments, and one moderate one: Docstring at forcedImage.py:242 about ForcedMeasurementTask needing to be subclassed for CCDs and Coadds is now obsolete. Also in ForcedMeasurementTask : you should always use **kwds in a Task 's __init__ , and pass those to Task.__init__ . Leave it to that to handle the config , parentTask , and name arguments. In the TestForcedMeasurementTask , I think you can just remove the __init__ method entirely (or maybe you could just remove the class?) The "expregion" and "newsource" variables in generateSources() should be "expRegion" and "newSources", respectively, to comply with LSST code standards. What do you think about moving generateSources() to ProcessImageForcedTask ? ProcessImageForcedTask.run() could then call generateSources() , and then pass the SourceCatalog to be filled to ForcedMeasurementTask . This would make ForcedMeasurementTask a little more similar to SingleFrameMeasurementTask , and then generateSources() could then call makeIdFactory() itself.
          Hide
          jbosch Jim Bosch added a comment -

          Also, new datasets in obs_lsstSim look fine. I don't think there really is much of a "right place" for them within the file.

          Show
          jbosch Jim Bosch added a comment - Also, new datasets in obs_lsstSim look fine. I don't think there really is much of a "right place" for them within the file.
          Hide
          pgee Perry Gee added a comment -

          I don't really understand how the kwds convention works, I am afraid. I added the extra arguments to _init_ just to satisfy the pipe_base code which I never understood. Perhaps at some point you can explain what sets the requirements for the input parameters on both Task and CmdTask.

          You last comment is actually the refactor struggle from last week which caused me to not be done. I originally wanted generateSources to be higher up to make the idFactory need easier to satisfy. When I did it that way, it also meant that the mapper.getOutputSchema() needs to we available to ProcessImageForcedTask prior to run. I didn't much like that, but I suppose that I can have a method like ForcedMeasurementTask.getOutputSchema() which could be called prior to init. I guess we should have finished chatting this issue out last week.

          Please send me your opinion before 9:00 tonight (Sunday) and I can try to implement this evening.

          Show
          pgee Perry Gee added a comment - I don't really understand how the kwds convention works, I am afraid. I added the extra arguments to _ init _ just to satisfy the pipe_base code which I never understood. Perhaps at some point you can explain what sets the requirements for the input parameters on both Task and CmdTask. You last comment is actually the refactor struggle from last week which caused me to not be done. I originally wanted generateSources to be higher up to make the idFactory need easier to satisfy. When I did it that way, it also meant that the mapper.getOutputSchema() needs to we available to ProcessImageForcedTask prior to run. I didn't much like that, but I suppose that I can have a method like ForcedMeasurementTask.getOutputSchema() which could be called prior to init. I guess we should have finished chatting this issue out last week. Please send me your opinion before 9:00 tonight (Sunday) and I can try to implement this evening.
          Hide
          jbosch Jim Bosch added a comment -

          The kwds are just a way of forwarding function arguments from one call to another. In this case, the extra task arguments that need to be passed to the base class must be present because they're passed by the makeSubtask() and parseAndRun() methods, and the base take constructor needs them to work properly.

          I'd be OK with resolving the issue with moving generateSources() by just having it refer to self.measurement.mapper.getOutputSchema(), or by having ForcedMeasurementTask set self.schema to that in its ctor and having generateSources() refer to self.measurement.schema.

          Show
          jbosch Jim Bosch added a comment - The kwds are just a way of forwarding function arguments from one call to another. In this case, the extra task arguments that need to be passed to the base class must be present because they're passed by the makeSubtask() and parseAndRun() methods, and the base take constructor needs them to work properly. I'd be OK with resolving the issue with moving generateSources() by just having it refer to self.measurement.mapper.getOutputSchema(), or by having ForcedMeasurementTask set self.schema to that in its ctor and having generateSources() refer to self.measurement.schema.
          Hide
          pgee Perry Gee added a comment -

          Personally, I think that a design where the subtask is in charge of knowing about the source schema is a better design. On the theory that the calling class doesn't need to have this information, but the subtask does.

          However, I'm all for consistency, since measurement.py and sfm.py are already built supplying the schema and measCat.

          I think I would prefer to do it the way it is done in sfm, using a writable schema variable.

          Show
          pgee Perry Gee added a comment - Personally, I think that a design where the subtask is in charge of knowing about the source schema is a better design. On the theory that the calling class doesn't need to have this information, but the subtask does. However, I'm all for consistency, since measurement.py and sfm.py are already built supplying the schema and measCat. I think I would prefer to do it the way it is done in sfm, using a writable schema variable.
          Hide
          jbosch Jim Bosch added a comment -

          I think that's a fair characterization: the way we're doing this in all the process tasks is a little ugly, because the parent tasks always need to know the schema that's defined mostly by child tasks, but it'd take a huge amount of work to change that across the board, and it's better to maintain consistency for now.

          Show
          jbosch Jim Bosch added a comment - I think that's a fair characterization: the way we're doing this in all the process tasks is a little ugly, because the parent tasks always need to know the schema that's defined mostly by child tasks, but it'd take a huge amount of work to change that across the board, and it's better to maintain consistency for now.
          Hide
          ajc Andrew Connolly added a comment -

          Accident

          Show
          ajc Andrew Connolly added a comment - Accident

            People

            Assignee:
            pgee Perry Gee
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Jim Bosch
            Watchers:
            Andrew Connolly, Jim Bosch, Perry Gee
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.