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

DM-21221 broke cp_pipe due to lack of tests

    Details

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

      Description

      The changing of ...Nl to ...nonLinearity is an API change which breaks things, including but not limited to makeBrighterFatterKernel due to a lack of tests (i.e. it's not really Andrés Alejandro Plazas Malagón's fault.

      Also, the API change where that method doesn't return dataset and now just does return is breaking too (also due to lack of tests).

      It is deliberately ambiguous whether this ticket is for fixing the breakages, or doing that and adding tests.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            I tried running the code, and agree that it runs.

            However, I am confused about the ticket. It says:

            the API change where that method doesn't return dataset and now just does return is breaking too

            I don't understand why this would cause breakage. The only (non-test) place which calls fitPtcAndNonLinearity already has dataset in scope. What was the breakage here? Indeed, I made this change

            --- a/python/lsst/cp/pipe/ptc.py
            +++ b/python/lsst/cp/pipe/ptc.py
            @@ -354,7 +354,8 @@ class MeasurePhotonTransferCurveTask(pipeBase.CmdLineTask):
                     lookupTableArray = np.zeros((numberAmps, numberAduValues), dtype=np.float32)
             
                     # Fit PTC and (non)linearity of signal vs time curve, produce linearizer
            -        dataset = self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType)
            +        #dataset =
            +        self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType)
             
                     if self.config.makePlots:
                         self.plot(dataRef, dataset, ptcFitType=self.config.ptcFitType)
            @@ -830,7 +831,7 @@ class MeasurePhotonTransferCurveTask(pipeBase.CmdLineTask):
                         dataset.nonLinearityResiduals[ampName] = linResidualNonLinearity
                         dataset.coefficientLinearizeSquared[ampName] = c0
             
            -        return dataset
            +#        return dataset
             
                 def plot(self, dataRef, dataset, ptcFitType):
                     dirname = dataRef.getUri(datasetType='cpPipePlotRoot', write=True)
            

            and everything seems to work fine. Can somebody explain the issue?

            Also also: we talked about how difficult this is to test. However, there are multiple calls to fitPtcAndNonLinearity in the tests directory. Any one (or all) of those could be checking the return value. Could we add that on this ticket?

            Show
            swinbank John Swinbank added a comment - I tried running the code, and agree that it runs. However, I am confused about the ticket. It says: the API change where that method doesn't return dataset and now just does return is breaking too I don't understand why this would cause breakage. The only (non-test) place which calls fitPtcAndNonLinearity already has dataset in scope. What was the breakage here? Indeed, I made this change --- a/python/lsst/cp/pipe/ptc.py +++ b/python/lsst/cp/pipe/ptc.py @@ -354,7 +354,8 @@ class MeasurePhotonTransferCurveTask(pipeBase.CmdLineTask): lookupTableArray = np.zeros((numberAmps, numberAduValues), dtype=np.float32)   # Fit PTC and (non)linearity of signal vs time curve, produce linearizer - dataset = self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType) + #dataset = + self.fitPtcAndNonLinearity(dataset, lookupTableArray, ptcFitType=self.config.ptcFitType)   if self.config.makePlots: self.plot(dataRef, dataset, ptcFitType=self.config.ptcFitType) @@ -830,7 +831,7 @@ class MeasurePhotonTransferCurveTask(pipeBase.CmdLineTask): dataset.nonLinearityResiduals[ampName] = linResidualNonLinearity dataset.coefficientLinearizeSquared[ampName] = c0   - return dataset +# return dataset   def plot(self, dataRef, dataset, ptcFitType): dirname = dataRef.getUri(datasetType='cpPipePlotRoot', write=True) and everything seems to work fine. Can somebody explain the issue? Also also: we talked about how difficult this is to test. However, there are multiple calls to fitPtcAndNonLinearity in the tests directory. Any one (or all) of those could be checking the return value. Could we add that on this ticket?
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Given I couldn't run it at the time, those were guesses as to what I imagined might have been broken. If it was never broken, all the better. Sorry for not being explicit about what was assumed and what was tested.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Given I couldn't run it at the time, those were guesses as to what I imagined might have been broken. If it was never broken, all the better. Sorry for not being explicit about what was assumed and what was tested.
            Hide
            swinbank John Swinbank added a comment -

            Looks good

            Show
            swinbank John Swinbank added a comment - Looks good
            Hide
            swinbank John Swinbank added a comment -

            Oh, but since you haven't modified the tests as I suggested above, could you please file a new ticket to capture that before closing this one?

            Show
            swinbank John Swinbank added a comment - Oh, but since you haven't modified the tests as I suggested above, could you please file a new ticket to capture that before closing this one?
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment -

            OK!; DM-23681

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - OK!; DM-23681

              People

              • Assignee:
                plazas Andrés Alejandro Plazas Malagón
                Reporter:
                mfisherlevine Merlin Fisher-Levine
                Reviewers:
                John Swinbank
                Watchers:
                Andrés Alejandro Plazas Malagón, Christopher Waters, John Swinbank, Merlin Fisher-Levine
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel