DM-50039: Update getTemplate unit tests #443
Conversation
abudlong
changed the title
abudlong
force-pushed
the
tickets/DM-50039
branch
from
1dd11a7 to
884fe9a
Compare
bsmartradio
left a comment
bsmartradio
left a comment
There was a problem hiding this comment.
Great job on the complex unit testing! Most of my comments are about docstring formatting, though there is one area of duplicated code that shows up a number of times in the tests that you may want to turn into a function if possible. Also thank you for the comments, they helped me follow the workflow of the tests.
| # Not checking the mask, as warping changes the sizes of the masks. | ||
|
|
||
| def testRunQuantum(self): | ||
| """ |
There was a problem hiding this comment.
One tiny nitpick on the docstrings, the standard in this file is for the docstrings to start on the same line as the """, not on the line below them. You do this in the context manager, but all of the following tests have it on a seperate line. Another tiny thing, could you add what you expect test runQuantum to do in the docstring? Pass/succeed etc. You do this well in most of the other unit tests, and I know we inconsistently list the success conditions in some of the other unit tests, but its a good idea.
|
|
||
| def testGetExposuresWithOverlap(self): | ||
| """ | ||
| Test getExposures with an overlapping patch exercises the intersection |
There was a problem hiding this comment.
Could you add the expected outcome of the test here?
| self.expectedNoDataSum = 0 | ||
| # self.bandwidth = 147.0 | ||
| config.effectiveWavelength = 478.5 | ||
| # self.effectiveWavelength = 478.5 |
There was a problem hiding this comment.
Do we still need these self comments here? If they aren't used we can remove them.
| butlerQC.put.assert_called_once_with("final_output", outputRefs) | ||
|
|
||
| def testValidate(self): | ||
| # Check that function does not raise ValueError when |
There was a problem hiding this comment.
Should this be a docstring rather than a comment?
| self.dataIds[tract.tract_id].append(dataCoordinate) | ||
|
|
||
| def testConnectionsRemovesCoaddExposures(self): | ||
| """ |
There was a problem hiding this comment.
Again a slight change to the docstrings. If you want to check the style guide, https://developer.lsst.io/python/numpydoc.html, it shows the expected docstring style.
| assert expected in dcr_connections.inputs, f"Expected '{expected}' missing in DCR connections" | ||
|
|
||
| def testDcrRunQuantum(self): | ||
| """ |
There was a problem hiding this comment.
Docstring update, and possibly add that this is expected to pass?
| """ | ||
|
|
||
| # Create a local task instance | ||
| config = GetDcrTemplateTask.ConfigClass() |
There was a problem hiding this comment.
So, I notice that this gets set in most of the tests, even though these are also set in the setup? Is this because you need a local instance and can't use the setup? Is it possible to make a small function which is something like local_setup so that the code isn't repeated in each setup? If its not possible don't worry, I know mocking complicates things.
| Test that checkPatchList correctly validates the number of DCR | ||
| subfilters per patch. | ||
| Setup: | ||
| - For each tract in self.patches, create a list of patch handles. | ||
| - Each handle is repeated once for each expected DCR subfilter. | ||
| This simulates having multiple DCR-matched exposures per patch, | ||
| one for each subfilter. | ||
| Expected behavior: | ||
| - If the number of patch entries matches task.config.numSubfilters, | ||
| checkPatchList passes. | ||
| - If a patch has too few or too many entries, checkPatchList raises a | ||
| RuntimeError. | ||
| """ |
There was a problem hiding this comment.
I was looking through the style guide, and I'm not sure we have an equivalent style for the setup/behavior. I suppose if you reformat this to look like an example this would work if this should be in the docstrings? I don't know how to link to the exact section but search for examples. https://developer.lsst.io/python/numpydoc.html
| config.numSubfilters = 3 | ||
| task = taskClass(config=config) | ||
|
|
||
| # 1. Prepare a patch list with correct number of subfilters |
There was a problem hiding this comment.
This is the only comments which have numbering? If I understand it correctly, the numbers match with the expected behavior listed above? If that's the case, maybe add the numbers there to make it clear they map together?
No description provided.