-
Notifications
You must be signed in to change notification settings - Fork 14
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-46239: Add pixelization envelopes for Intersection/UnionRegion #72
Conversation
66a4a0f
to
18ca827
Compare
5f2c428
to
4af5d3f
Compare
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.
First: a typo and a leftover flatten. Second: I'm worried that findPixels is recursive, I thought that there was discussion on the ticket that this is bad? (This may be way out of my wheelhouse though).
tests/test_HtmPixelization.py
Outdated
rsu2 = pixelization.envelope(union2) | ||
self.assertEqual(rsu2, rsu2 | rsu) | ||
|
||
# CHeck with intersection |
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.
Typo in comment.
Allowing to loop over a flattened structures, we wanted to push to another ticket when it's clearer how to define a better API. |
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.
Looks good. I wish we had an alternative to a getRegions
that deep-copies everything just to let Python iterate over it, but given pybind11's inflexibility re unique_ptr
vs. shared_ptr
holders for the same type, I imagine we're pretty much out of options.
I think we're fine with letting this continue to be a fully recursive implementation and punt flattening that out to a new ticket. |
1302ec6
to
d7018da
Compare
d7018da
to
d008b96
Compare
Checklist
doc/changes