DM-24731: Create a unit test framework for AssembleCoaddTask #393
Conversation
There was a problem hiding this comment.
The new MockCoaddTestData object seems useful enough that it could live in pipe/tasks/mocks: it looks like it might be a more specific coadd mocking system than the existing ones.
It's not clear to me that the gen2/gen3 part of this is testing what you think it's testing. Or maybe I'm confused about what is being done here. Your tests call runDataRef and runQuantum, but your modified versions of appear to be not at all different, and do not follow the API for those methods.
That the new files have mostly un-namespaced imports bothers me, but it seems I've de facto lost that argument?
| """Very simple object that looks like a Gen2 data reference to an exposure. | ||
|
|
||
| Attributes | ||
| ---------- |
There was a problem hiding this comment.
If you're going to go this route, you also need a Parameters section, since this is missing the exposure argument, but also has things that aren't arguments. In fact, I don't see an Attributes section in our python doc guide at all:
https://developer.lsst.io/python/numpydoc.html#documenting-constants-and-class-attributes
Given that tests do not get built documentation, I don't think it's worth doing the full Parameters+Attributes documentation per the dev guide, since most of these parameters become attributes. Also, since these are mocks, they're supposed to look like a specific object, so you could just reference that object, if you're not using unittest.mock.
There was a problem hiding this comment.
I wanted to document the class attributes, since I personally find that very helpful. The Attributes section is part of numpydoc even if it's not in our python doc guide, but I can change this to a Parameters section and move the remaining docstrings to in-line comments where the attributes are defined.
There was a problem hiding this comment.
If you want to use the inline comment method, you need to specify those attributes as class attributes, outside of __init__ (default value None if nothing else suits).
https://developer.lsst.io/python/numpydoc.html#attributes-set-in-init-methods
That is correct, the modified versions of |
isullivan
force-pushed
the
tickets/DM-24731
branch
from
da7d5ab
to
52999ab
Compare
| """Very simple object that looks like a Gen2 data reference to an exposure. | ||
|
|
||
| Attributes | ||
| ---------- |
There was a problem hiding this comment.
If you want to use the inline comment method, you need to specify those attributes as class attributes, outside of __init__ (default value None if nothing else suits).
https://developer.lsst.io/python/numpydoc.html#attributes-set-in-init-methods
tests/assembleCoaddTestUtils.py
Outdated
| raise ValueError(f"A bbox must be supplied for dataset {datasetType}") | ||
| else: | ||
| if bbox is not None: | ||
| raise ValueError(f"A bbox cannot be supplied for dataset {datasetType}") |
There was a problem hiding this comment.
Is this what happens when you supply bbox to a real butler dataRef?
tests/assembleCoaddTestUtils.py
Outdated
| @@ -318,36 +323,62 @@ def __init__(self, shape=geom.Extent2I(201, 301), offset=geom.Point2I(-123, -45) | |||
| self.ra = ra | |||
| self.dec = dec | |||
| self.rotAngle = 0.*degrees | |||
| "Rotation of the pixel grid on the sky, East from North (`lsst.geom.Angle`)." | |||
There was a problem hiding this comment.
As above, all of these pseudo-docstrings need to be done via class member declarations and docstrings: https://developer.lsst.io/python/numpydoc.html#attributes-set-in-init-methods
| supplementaryData = self.makeSupplementaryData(mockSkyInfo, warpRefList=inputs.tempExpRefList) | ||
|
|
||
| retStruct = self.run(mockSkyInfo, inputs.tempExpRefList, inputs.imageScalerList, | ||
| inputs.weightList, supplementaryData=supplementaryData) | ||
| return retStruct | ||
|
|
||
| def runDataRef(self, mockSkyInfo, selectDataList=None, warpRefList=None): | ||
| """Simplified API for testing image coaddition algorithms. | ||
| """Modified interface for testing coaddition algorithms without a Butler. |
There was a problem hiding this comment.
Thank you, that's better wording.
There was a problem hiding this comment.
Thanks for the clarification on the Mock*CoaddTask docstrings: their purpose is now clearer. Also, the checkGen2Gen3 test code makes those tests a lot easier to follow.
You need to make class members to get attribute docstrings. I've given a link to our dev guide on that.
This makes writing unit tests easier because getPsf() is used elsewhere.
isullivan
force-pushed
the
tickets/DM-24731
branch
from
52999ab
to
ebe61ba
Compare
|
I've done as you asked for the attribute docstrings. All I really wanted, though, was simple code comments to help future people out who were looking at the code, since docs don't get built for tests. |
…asses. Only very simple unit tests have been added at this point, to demonstrate usage.
isullivan
force-pushed
the
tickets/DM-24731
branch
from
ebe61ba
to
cfff39a
Compare
This creates mock data references that mimic the API of the Gen 2 and Gen 3 butler in order to pass simulated test data to AssembleCoaddTask without having to instantiate a real Butler. I have only written fairly basic unit tests to demonstrate their use, but they exercise most of the coadd assembly code and work through the difficult part of getting the metadata set up properly.