Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-40517: add unit test for quickLook #77

Merged
merged 9 commits into from
Jan 27, 2024
Merged

DM-40517: add unit test for quickLook #77

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8f49355
Check existance of bfKernel
isullivan Dec 14, 2023
c5f7820
Add basic unit test for quickLook
isullivan Dec 14, 2023
280fc3c
Pass through all additional keywords from IsrTask to Quicklook
isullivan Jan 22, 2024
08c18db
save work
isullivan Jan 23, 2024
a9c4aa4
Add keywords to quickLook
isullivan Jan 26, 2024
549e6c2
Pass IsrTask as keyword to quickLook init so it can be used in unit t…
isullivan Jan 26, 2024
c780214
Add unit test of quickLook.runQuantum
isullivan Jan 26, 2024
2c2e7ee
Actually load fringe data in quickLook
isullivan Jan 26, 2024
0a50ce5
Allow misconfigured newBFKernel to raise an exception.
isullivan Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 35 additions & 7 deletions python/lsst/summit/utils/quickLook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,22 +89,32 @@ class QuickLookIsrTask(pipeBase.PipelineTask):
ConfigClass = QuickLookIsrTaskConfig
_DefaultName = "quickLook"

def __init__(self, **kwargs):
def __init__(self, isrTask=IsrTask, **kwargs):
super().__init__(**kwargs)
# Pass in IsrTask so that we can modify it slightly for unit tests.
self.isrTask = IsrTask
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This took me longer to understand than I care to admit, so I think a slightly more thorough comment might be useful here. Perhaps something along the lines of

        # Pass in IsrTask so that we can modify it slightly for unit tests.
        # Note that this is not an instance of the IsrTask class, but the class
        # itself, which is then instantiated later on, in the run() method,
        # with the dynamically generated config.


def run(self, ccdExposure, *,
camera=None,
bias=None,
dark=None,
flat=None,
fringes=pipeBase.Struct(fringes=None),
defects=None,
linearizer=None,
crosstalk=None,
bfKernel=None,
bfGains=None,
newBFKernel=None,
ptc=None,
crosstalkSources=None,
isrBaseConfig=None
isrBaseConfig=None,
filterTransmission=None,
opticsTransmission=None,
strayLightData=None,
sensorTransmission=None,
atmosphereTransmission=None,
deferredChargeCalib=None,
illumMaskedImage=None,
):
"""Run isr and cosmic ray repair using, doing as much isr as possible.

Expand Down Expand Up @@ -224,7 +234,17 @@ def run(self, ccdExposure, *,
isrConfig.doCrosstalk = True
self.log.info("Running with crosstalk correction")

if bfKernel:
if newBFKernel is not None:
try:
bfGains = newBFKernel.gain
isrConfig.doBrighterFatter = True
self.log.info("Running with new brighter-fatter correction")
except AttributeError:
bfGains = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a newBFKernel is supplied but we fail to pull the gains out then I'd think we'd probably want to at least log a warning here. What's your thinking on why this shouldn't be a raise though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I took the statement "Should never fail" in the docstring of this task too literally! I'm happy to either log a warning or let it raise, your choice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I see, I did wonder if it was something like that. I mean, it should never fail based if valid things are being passed in, but if, say, a calib was the wrong size or something truly awful like that then I think in those cases it should still raise tbh, otherwise the behaviour is sort of ill-defined. Everything that's passed in should work, and if it doesn't then I think that's raise-time. All the other raises like that are implicit and come from isrTask raising though, so I think raising here is fine.

else:
bfGains = None

if bfKernel is not None and bfGains is None:
isrConfig.doBrighterFatter = True
self.log.info("Running with brighter-fatter correction")

Expand All @@ -233,20 +253,28 @@ def run(self, ccdExposure, *,
self.log.info("Running with ptc correction")

isrConfig.doWrite = False
isrTask = IsrTask(config=isrConfig)
isrTask = self.isrTask(config=isrConfig)
result = isrTask.run(ccdExposure,
camera=camera,
bias=bias,
dark=dark,
flat=flat,
# fringes=pipeBase.Struct(fringes=None),
fringes=fringes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're actually accepting the fringes if they're passed in, I think we should also have a block that tests if we got fringes supplied, and if so, sets the config option on to use them. We can then remove the # TODO: deal with fringes here comment too 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just replacing that TODO with

        if fringes.fringes is not None:
            isrConfig.doFringe = True
            self.log.info("Running with fringe correction")

should do it.

defects=defects,
linearizer=linearizer,
crosstalk=crosstalk,
bfKernel=bfKernel,
bfGains=bfGains,
ptc=ptc,
crosstalkSources=crosstalkSources,)
crosstalkSources=crosstalkSources,
filterTransmission=filterTransmission,
opticsTransmission=opticsTransmission,
sensorTransmission=sensorTransmission,
atmosphereTransmission=atmosphereTransmission,
strayLightData=strayLightData,
deferredChargeCalib=deferredChargeCalib,
illumMaskedImage=illumMaskedImage,
)

postIsr = result.exposure

Expand Down