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

IsrTask is not a valid CmdLineTask

    Details

    • 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.

        Attachments

          Issue Links

            Activity

            Hide
            czw 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
            czw 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
            price 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
            price 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
            czw 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
            czw 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
            czw 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
            czw 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
            czw 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.

            Show
            czw 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:
                czw Christopher Waters
                Reporter:
                jbosch 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
              • Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel