XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
18
• Sprint:
DRP F17-1, DRP F18-4
• Team:
Data Release Production

#### Description

IsrTask is a command-line task, but its run method does not take a dataRef (it instead has a runDataRef method. This is inconsistent with other CmdLineTasks and more importantly breaks parseAndRun.

I'm committing a small workaround on DM-6631 to get parseAndRun working, but the ultimately method names should be made consistent across CmdLineTasks. That will require an API change and hence an RFC.

#### Activity

Hide
Christopher Waters added a comment -

The IsrTask is called as a subtask to ProcessCcdTask everywhere it is invoked.  This allows it to be retargeted to the particular camera as needed.  This is not the case when called from a dedicated runIsr.py script.  IsrTask is then the main task, and cannot be retargeted.

I am looking for suggestions, as my initial idea of creating a dummy task that does nothing but setup and retarget IsrTask as a subtask does not feel like an elegant solution.

Show
Christopher Waters added a comment - The IsrTask is called as a subtask to ProcessCcdTask everywhere it is invoked.  This allows it to be retargeted to the particular camera as needed.  This is not the case when called from a dedicated runIsr.py script.  IsrTask is then the main task, and cannot be retargeted. I am looking for suggestions, as my initial idea of creating a dummy task that does nothing but setup and retarget IsrTask as a subtask does not feel like an elegant solution.
Hide
Paul Price added a comment -

I think you need a wrapper class. It may not be completely elegant, but it's the way it needs to be done.

 class IsrWrapperConfig(Config):  isr = ConfigurableField(target=IsrTask, doc="Instrument signature removal")   class IsrWrapperTask(CmdLineTask):  ConfigClass = IsrWrapperConfig  def __init__(self, *args, **kwargs):  super().__init__(*args, **kwargs)  self.makeSubtask("isr")  def runDataRef(self, dataRef):  return self.isr.runDataRef(dataRef) 

If you don't like that, you could stick it in pipe_drivers and have it mirror singleFrameDriver.py (isrDriver.py?), which would not just make it retargetable, but also allow you to spread it over multiple nodes.

Show
Paul Price added a comment - I think you need a wrapper class. It may not be completely elegant, but it's the way it needs to be done. class IsrWrapperConfig(Config): isr = ConfigurableField(target=IsrTask, doc="Instrument signature removal")   class IsrWrapperTask(CmdLineTask): ConfigClass = IsrWrapperConfig def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.makeSubtask("isr") def runDataRef(self, dataRef): return self.isr.runDataRef(dataRef) If you don't like that, you could stick it in pipe_drivers and have it mirror singleFrameDriver.py ( isrDriver.py ?), which would not just make it retargetable, but also allow you to spread it over multiple nodes.
Hide
Christopher Waters added a comment -

I've written the wrapper script, and done tests with HSC data to confirm that it does something reasonable.  I've also added configuration files for the other obs_* packages.  Are there datasets and calib data already defined for these other cameras to allow them to be tested as well?

Show
Christopher Waters added a comment - I've written the wrapper script, and done tests with HSC data to confirm that it does something reasonable.  I've also added configuration files for the other obs_* packages.  Are there datasets and calib data already defined for these other cameras to allow them to be tested as well?
Hide
Christopher Waters added a comment -

The addition of a CmdLineTask for ISR has prompted a number of configuration changes in each obs_ package to enable both runIsr.py and processCcd.py to read the same information.

This wasn't set as "In review" due to mistakes on my part.

Show
Christopher Waters added a comment - The addition of a CmdLineTask for ISR has prompted a number of configuration changes in each obs_ package to enable both runIsr.py and processCcd.py to read the same information. This wasn't set as "In review" due to mistakes on my part.
Hide
Christopher Waters added a comment -

I've tried to split up the reviewing of the obs_* packages to people with some familiarity with the data (even if it was me taking github's advice).  The ip_isr package has the most changes that are not simply splitting configuration files, and is probably helpful to understand the reason for those splits, and so I welcome comments on that package from everyone.

If anyone wants to look over any of the other packages, I'd be happy to read those comments.  I simply didn't want any one person to be surprised with a dozen sets of changes to review.

A list of package/github reviewers:

ip_isr: Jim Bosch, Paul Price, Yusra AlSayyad, Meredith Rawls, Merlin Fisher-Levine, Merlin Fisher-Levine, Dominique Boutigny
obs_base: John Parejko
obs_cfht: Dominique Boutigny
obs_ctio0m9: Merlin Fisher-Levine
obs_decam: Meredith Rawls
obs_lsstCam: Merlin Fisher-Levine
obs_lsstSim: John Parejko
obs_monocam: Paul Price
obs_subaru: Paul Price
obs_test: Jim Bosch

Sorry for any confusion.  The logistics on this ticket review are more complicated than I've worked with before.

Show
Christopher Waters added a comment - I've tried to split up the reviewing of the obs_* packages to people with some familiarity with the data (even if it was me taking github's advice).  The ip_isr package has the most changes that are not simply splitting configuration files, and is probably helpful to understand the reason for those splits, and so I welcome comments on that package from everyone. If anyone wants to look over any of the other packages, I'd be happy to read those comments.  I simply didn't want any one person to be surprised with a dozen sets of changes to review. A list of package/github reviewers: ip_isr: Jim Bosch , Paul Price , Yusra AlSayyad , Meredith Rawls , Merlin Fisher-Levine , Merlin Fisher-Levine , Dominique Boutigny obs_base: John Parejko obs_cfht: Dominique Boutigny obs_ctio0m9: Merlin Fisher-Levine obs_decam: Meredith Rawls obs_lsstCam: Merlin Fisher-Levine obs_lsstSim: John Parejko obs_monocam: Paul Price obs_sdss: Yusra AlSayyad obs_subaru: Paul Price obs_test: Jim Bosch Sorry for any confusion.  The logistics on this ticket review are more complicated than I've worked with before.

#### People

Assignee:
Christopher Waters
Reporter:
Jim Bosch
Reviewers:
Meredith Rawls, Merlin Fisher-Levine, Paul Price, Yusra AlSayyad
Watchers:
Christopher Waters, Hsin-Fang Chiang, Jim Bosch, John Parejko, Meredith Rawls, Merlin Fisher-Levine, Robert Lupton, Yusra AlSayyad