-
Notifications
You must be signed in to change notification settings - Fork 45
Point In Face & Get Faces Containing Point #1056
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
Conversation
|
@ philipc2 do you think this should be an internal function or exposed to the user? |
philipc2
left a comment
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 include an ASV benchmark. I'd suggest doing a parameterized benchmark for the 120 and 480 km MPAS grids.
philipc2
left a comment
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 use the optimized functions from #1072 and try to write the function entirely in Numba. This may require us to pass in both the cartesian and spherical versions of point & polygon. Let me know if you have any questions!
philipc2
left a comment
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 add a couple points in the "Overview" of the description of this PR.
Done! It made me realize just how big this PR has gotten! |
rajeeja
left a comment
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.
Resolve comments.
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.
For the notebook:
- Add an introduction below the title consistent with how other notebooks are formatted.
- I don't think we should be showcasing the
point_in_facefunction from the start. We should highlight theGridfunctionality. I think we can restructure the notebook to remove thepoint_in_facepart - Consider adding intermediate markdown blocks between each cell and try to avoid the inline comments, these should be in Markdown
Removing the |
Maybe we can hold off on the notebook for right now, and keep the examples contained within the docstring? |
|
Using this syntax/style would be good |
Ok! Yeah, I think this works better. |
Co-authored-by: Philip Chmielowiec <[email protected]>
Co-authored-by: Philip Chmielowiec <[email protected]>
philipc2
left a comment
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.
Excellent work!
Co-authored-by: Philip Chmielowiec <[email protected]>


Adds several new features:
point_in_facefunction to determine if a point is in a face.get_faces_containing_pointwhich given a point finds the face (or faces) the point resides in.max_face_radiuswhich finds the largest face radius and stores it within the grid.haversine_distancefunction which finds the haversine distance between two given points.Closes #905, #1141
Overview
Expected Usage
PR Checklist
General
Testing
Documentation
_) and have been added todocs/internal_api/index.rstdocs/user_api/index.rst