DM-40517: add unit test for quickLook #77
Conversation
isullivan
force-pushed
the
tickets/DM-40517
branch
from
900591d
to
dc32364
Compare
There was a problem hiding this comment.
This looks really great as a set of tests in general, and I think is really worth adding! There's one moderately-sized problem though: the issue before was in the actual command line task construction & execution (the connection magic), and this still doesn't get that tested with these changes. I reverted Jim's fix on DM-40512 to check, and the task then fails on the command line, but these tests still pass. I'm not sure exactly of the best way of testing this, but I thought that perhaps a SimplePipelineExector might be the way to go, as this will run it via the same machinery as the command line does.
isullivan
force-pushed
the
tickets/DM-40517
branch
3 times, most recently
from
8e69d20
to
b058434
Compare
Since it's an array, we can't check existance with an if statement
isullivan
force-pushed
the
tickets/DM-40517
branch
2 times, most recently
from
a0d71df
to
6255483
Compare
isullivan
force-pushed
the
tickets/DM-40517
branch
from
6255483
to
c780214
Compare
|
I added a unit test that sets up a test butler with the necessary datasets, and which fails with the unmodified |
| except AttributeError: | ||
| bfGains = None |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| result = isrTask.run(ccdExposure, | ||
| camera=camera, | ||
| bias=bias, | ||
| dark=dark, | ||
| flat=flat, | ||
| # fringes=pipeBase.Struct(fringes=None), | ||
| fringes=fringes, |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
| # Pass in IsrTask so that we can modify it slightly for unit tests. | ||
| self.isrTask = IsrTask |
There was a problem hiding this comment.
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.
tests/test_quickLook.py
Outdated
|
|
||
|
|
||
| class QuickLookIsrTaskTestCase(unittest.TestCase): | ||
|
|
| linearizer = "linearizer" | ||
| fringes = "fringe" | ||
| sensorTransmission = "transmission_sensor" | ||
| crosstalkSources = "isrOverscanCorrected" |
There was a problem hiding this comment.
Is that true?! I mean, I don't doubt you at all, but that's a horrible name mapping!
There was a problem hiding this comment.
tests/test_quickLook.py
Outdated
| butlerTests.addDataIdValue(self.repo, "instrument", instrument) | ||
| butlerTests.addDataIdValue(self.repo, "physical_filter", physical_filter, band=band) | ||
| butlerTests.addDataIdValue(self.repo, "detector", detector) | ||
| # butlerTests.addDataIdValue(self.repo, "detector", detector + 1) |
There was a problem hiding this comment.
Should this commented line be removed? Or was the plan to iterate over this option and check it raises for a detector id mismatch?
tests/test_quickLook.py
Outdated
| # Turn on all optional inputs | ||
| config.doAttachTransmissionCurve = True | ||
| config.doIlluminationCorrection = True | ||
| config.doStrayLight = True | ||
| config.doDeferredCharge = True | ||
| config.usePtcReadNoise = True | ||
| config.doCrosstalk = True | ||
| config.doBrighterFatter = False |
There was a problem hiding this comment.
I'm a little confused by this block in two small ways: it says to turn on all option inputs, but it doesn't include many (mostly the main ones, like bias/dark/flat etc), and also config.doBrighterFatter is set to False whereas all the others are True. Could you just clarify?
There was a problem hiding this comment.
Good catch! I forgot to turn this back on after I got BF working.
isullivan
force-pushed
the
tickets/DM-40517
branch
from
4fd04ca
to
2c2e7ee
Compare
No description provided.