-
Notifications
You must be signed in to change notification settings - Fork 509
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
Implemented contains for BoundingBox containing other BoundingBox #2906
Implemented contains for BoundingBox containing other BoundingBox #2906
Conversation
@shimwell, do you want to look at this since it seems like you did the initial implementation for this. |
This looks like a useful feature to me. I've made a few comments for consideration but nothing major I would recommend one more test that checks a bounding box is in another bounding box when the first bounding box is only partly within. This would return false. |
Co-authored-by: Jonathan Shimwell <[email protected]>
Ok that test was added. I also added one for one of the vertices being coincident because the objects were right there. |
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 nice PR that improves on the existing contains method to supports bounding boxes, includes tests, doc strings updates, pep8 formatting. All looks good to me and I'm happy to merge this one in. Many thanks for this improved method
Will merge Tuesday afternoon (UK time) if there are no objections.
Thanks @shimwell. |
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'd like to take a look at this before we merge. Sorry to delay! Will get a review in asap
…enmc-dev#2906) Co-authored-by: Jonathan Shimwell <[email protected]>
Description
This is a quality of life feature request that tests if things are inside of a
BoundingBox
. Originally the goal was to haveif point in box
andif other_box in box
. Turns out since #2759,if point in box
has been implemented.This PR does the following:
if other_box in box
[1]*100 in box
gives a helpful error.if "foo" in box
gives a helpful error.__contains__
function forBoundingBox
to test it's accuracy, and the other tests it's error handling.It is unclear if this should be explicitly documented for users or not.
Fixes #2896
Checklist