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.
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
DM-35281: Avoid duplicate peak ID's #641
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of cleanups on the tests, but otherwise this looks good. Good catch on the bug, and I like the tests that demonstrate the problem.
I would suggest merging the test with the C++ commit: they go together.
tests/test_footprintMergeCatalog.py
Outdated
fp1.makeSources(self.catalog1) | ||
|
||
idFactory = afwTable.IdFactory.makeSimple() | ||
table = afwTable.SourceTable.make(schema, idFactory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're re-using the idFactory
name every time here, it might be clearer to just put the IdFactor.makeSimple()
inside the SourceTable.make()
call in each of these cases?
tests/test_footprintMergeCatalog.py
Outdated
# The peak catalog may be non-contiguous, | ||
# so add each peak individually | ||
peaks = [peak.getId() for peak in src.getFootprint().peaks] | ||
self.assertEqual(len(peaks), len(np.unique(peaks))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about self.assertEqual(sorted(peaks), sorted(set(peaks))
? That would specify exactly which ones don't match.
tests/test_footprintMergeCatalog.py
Outdated
del self.table | ||
|
||
def assertUniqueIds(self, catalog): | ||
"""Ensure that all of the peak IDs in a single parent are unique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Period at end of sentence.
No description provided.