Tickets/DM-47188: Fix bug where CalcZernikesTask fails if number of intra/extra stamps isn't equal #279
Conversation
| stampsExtra.extend([stampsExtra[0]]) | ||
|
|
||
| # Now estimate Zernikes | ||
| self.task.run(stampsExtra, stampsIntra) |
There was a problem hiding this comment.
So if it's supposed to stop can you test that indeed it's not going further? Or what would have happened if break wasn't there to test the new functionality.
There was a problem hiding this comment.
This test was added to reproduce the error, then the fix was written so this test passed.
Before, if e.g. there were more donuts in extraStamps than in intraStamps, it would hit a zk = None. This is because it had passed the number zernikes available. This happens because we are using zip_longest instead of zip, because this enables functionality in CalcZernikesUnpaired (where there are only intra or extra stamps, but not both). Hitting a zk = None caused an error because below it calls zk[j - 4], and None is not subscriptable.
Now, when it gets to zk = None it stops, because it knows it has iterated through every paired stamp.
| if "ENTROPY" in list(donutStamps.metadata): | ||
| entropyValue = np.asarray(donutStamps.metadata.getArray("ENTROPY")) | ||
| fillVals = np.asarray(donutStamps.metadata.getArray("ENTROPY")) |
There was a problem hiding this comment.
is this helping with the case where most of the donutStamps do not have entropy, but only some do? I'm wondering about the ordering (entropyValue has the same length as donutCatalog that corresponds to cutout donutStamps, so why would fillVals be shorter than that?
There was a problem hiding this comment.
It happens when you append a new DonutStamp to DonutStamps (like I do in the unit test I added). The DonutStamps append/extend methods don't extend the metadata at all, so e.g. donutStamps.metadata.getArray("ENTROPY") will return an array shorter than donutStamps.
We should probably create a ticket that fixes these methods so that metadata is properly carried along, however the append/extend methods are never used in the tasks, so it's low priority.
| # If zk is None, we need to stop. This can happen when running | ||
| # paired Zernike estimation and the number of intra/extra stamps | ||
| # is not the same | ||
| if zk is None: |
There was a problem hiding this comment.
So for one of the stamps not being there, we get None rather than [] ? I'm checking against
There was a problem hiding this comment.
Yes, simply because we are using zip_longest instead of zip. However, this only happens when running inside the paired task.
Interestingly, I think the method you link to there is a little outdated. It still functions totally fine, however its structure is leftover from before the paired and unpaired estimation methods became separate tasks. I think it could be simplified now. (although definitely not high priority since it works fine).
* Change zernikes output to QTable. * Add roundtrip for VisitInfo <-> dict * Remove VisitInfo from CutOutDonutsScienceSensorTask connections. Update addVisitInfoToCatTable test. * Add round trip dict to visit info test. * Add exposure time to visit_info dict. Add all keys to round trip test. * Add necessary metadata to tasks downstream of donutTables. * Update production pipeline for ComCam. * Enable RubinTV upload on production pipelines * Remove MJD assertion between intra and extra donut stamps. * tickets/DM-46117: single-side-of-focus pipeline (#273) * tickets/DM-47163: Cut on fraction of bad pixels; run stamp selection during CalcZernikeTask by default (#277) * Switch to new ISR task * Fixed bug where original mask bits aren't persisted in stamps. * Tickets/DM-47188: Fix bug where CalcZernikesTask fails if number of intra/extra stamps isn't equal (#279) * Increase stamp size in rapid analysis pipeline so donut edges aren't clipped (#280) --------- Co-authored-by: J. Bryce Kalmbach <[email protected]> Co-authored-by: Josh Meyers <[email protected]> Co-authored-by: Merlin Fisher-Levine <[email protected]>
No description provided.