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

Tickets/DM-47188: Fix bug where CalcZernikesTask fails if number of intra/extra stamps isn't equal #279

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Tickets/DM-47188: Fix bug where CalcZernikesTask fails if number of intra/extra stamps isn't equal #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bca4266
Made more robust to unequal length intra and extra stamp lists.
jfcrenshaw Oct 28, 2024
215c1c6
Bumped version history.
jfcrenshaw Oct 28, 2024
c0d4ea5
Linting.
jfcrenshaw Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions doc/versionHistory.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@
Version History
##################

.. _lsst.ts.wep-12.4.1:

-------------
12.4.1
-------------

* Fixed bug where CalcZernikesTask fails when the number of intra/extra stamps is not equal

.. _lsst.ts.wep-12.4.0:

-------------
Expand Down
6 changes: 6 additions & 0 deletions python/lsst/ts/wep/task/calcZernikesTask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,12 @@ def createZkTable(
zkCoeffCombined.flags,
)
):
# 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:
Copy link
Contributor

Choose a reason for hiding this comment

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

So for one of the stamps not being there, we get None rather than [] ? I'm checking against

ts_wep/python/lsst/ts/wep/task/estimateZernikesBase.py

Line 191 in 677cdd6

zkList = []

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

break

row = dict()
row["label"] = f"pair{i+1}"
row["used"] = not flag
Expand Down
13 changes: 9 additions & 4 deletions python/lsst/ts/wep/task/donutStampSelectorTask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,10 @@ def selectStamps(self, donutStamps):
entropySelect = np.ones(len(donutStamps), dtype="bool")

# Collect the entropy information if available
entropyValue = np.full(len(donutStamps), np.nan)
if "ENTROPY" in list(donutStamps.metadata):
entropyValue = np.asarray(donutStamps.metadata.getArray("ENTROPY"))
fillVals = np.asarray(donutStamps.metadata.getArray("ENTROPY"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

entropyValue[: len(fillVals)] = fillVals
if self.config.selectWithEntropy:
entropySelect = entropyValue < self.config.maxEntropy
else:
Expand All @@ -184,9 +186,10 @@ def selectStamps(self, donutStamps):
snSelect = np.ones(len(donutStamps), dtype="bool")

# collect the SN information if available
snValue = np.full(len(donutStamps), np.nan)
if "SN" in list(donutStamps.metadata):
snValue = np.asarray(donutStamps.metadata.getArray("SN"))

fillVals = np.asarray(donutStamps.metadata.getArray("SN"))
snValue[: len(fillVals)] = fillVals
if self.config.selectWithSignalToNoise:
# Use user defined SN cutoff or the filter-dependent
# defaults, depending on useCustomSnLimit
Expand All @@ -210,8 +213,10 @@ def selectStamps(self, donutStamps):
fracBadPixSelect = np.ones(len(donutStamps), dtype="bool")

# collect fraction-of-bad-pixels information if available
fracBadPix = np.full(len(donutStamps), np.nan)
if "FRAC_BAD_PIX" in list(donutStamps.metadata):
fracBadPix = np.asarray(donutStamps.metadata.getArray("FRAC_BAD_PIX"))
fillVals = np.asarray(donutStamps.metadata.getArray("FRAC_BAD_PIX"))
fracBadPix[: len(fillVals)] = fillVals
if self.config.selectWithFracBadPixels:
fracBadPixSelect = fracBadPix <= self.config.maxFracBadPixels
else:
Expand Down
12 changes: 12 additions & 0 deletions tests/task/test_calcZernikesTieTaskCwfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,3 +269,15 @@ def testWithAndWithoutPairs(self):
# Check that the averages are similar
zkAvgUnpaired = np.mean([zkAvgExtra, zkAvgIntra], axis=0)
self.assertLess(np.sqrt(np.sum(np.square(zkAvgPairs - zkAvgUnpaired))), 0.30)

def testUnevenPairs(self):
# Test for when you have more of either extra or intra
# Load the test data
stampsExtra = self.donutStampsExtra
stampsIntra = self.donutStampsIntra

# Increase length of extra list
stampsExtra.extend([stampsExtra[0]])

# Now estimate Zernikes
self.task.run(stampsExtra, stampsIntra)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Loading