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-13790: Remove all use of the geom package #19
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.
Commit message: "somebody" --> "someday"
For both PatchInfo and TractInfo replace getPolygon with getInnerSkyPolygon and getOuterSkyPolygon. Also add unit tests for the new methods Also add a new function makeSkyPolygonFromBBox, which is used to implement the new methods.
Two notes claimed that the vertex list in a TractInfo might someday become SphericalConvexPolygon but that will not happen. SphericalConvexPolygon is a class in the `geom` package, which we are no longer using.
I fixed the commit message |
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.
Please factor out the code duplication: four blocks are almost identical.
tests/test_dodecaSkyMap.py
Outdated
cornerShiftedIn = skyCorner.offset(bearing=bearingToCenter, amount=shiftAngle) | ||
cornerShiftedOut = skyCorner.offset(bearing=bearingToCenter, amount=-shiftAngle) | ||
self.assertTrue(outerPolygon.contains(cornerShiftedIn.getVector())) | ||
self.assertFalse(outerPolygon.contains(cornerShiftedOut.getVector())) |
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.
This is the same code as above for the inner polygon. Can you factor it into a function?
tests/test_dodecaSkyMap.py
Outdated
cornerShiftedIn = skyCorner.offset(bearing=bearingToCenter, amount=shiftAngle) | ||
cornerShiftedOut = skyCorner.offset(bearing=bearingToCenter, amount=-shiftAngle) | ||
self.assertTrue(outerPolygon.contains(cornerShiftedIn.getVector())) | ||
self.assertFalse(outerPolygon.contains(cornerShiftedOut.getVector())) |
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.
Ditto.
Thank you for another excellent suggestion. I centralized the polygon test code, which significantly reduced code duplication. I also did the following:
|
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.
Commit message for "Fix sorting bug in BaseSkyMap.findTract" has lines much too long.
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.
Very nice!
""" | ||
_SkyMapConfig = None | ||
def setAttributes(self, *, |
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.
I don't recognise this syntax. Is it py3? What's it mean?
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.
Yes it is py3; all arguments following a bare asterisk can only be specified by name.
self.numTracts = numTracts | ||
self.neighborAngularSeparation = neighborAngularSeparation | ||
self.numNeighbors = numNeighbors | ||
np.random.seed(47) |
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 did you choose 47? (:
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.
Pomona College had a thing about 47
`BaseSkyMap.findTract` assumed that all found distances would be different, so a list of (distance, tractInfo) could be sorted. Fixed by sorting on `(distance, index, tractInfo)` in order to break any degeneracy in distance.
So that all skymap test cases run it.
Update SkyMapTestCase to support sky maps with small numbers of tracts (e.g. EquatSkyMap). As part of this, start using numpy to randomly select patches to test. Thus SkyMapTestCase.setUp now needs to be reliably run so it can set the random number seed. I was reluctant to force subclasses to call super(setUp) since forgetting to do so would be an invisible error (it would silently fail to set the random seed) unless I added some special test. Instead I overhauled the way subclasses of SkyMapTestCase set parameters needed by the base class. The result is a significant improvement in safety, since a spelling error will be caught immediately. I think the new code is also more readable.
I fixed the far-too-long lines in the commit message for "Fix sorting bug in BaseSkyMap.findTract". |
No description provided.