Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes how grid faces containing a point are found by introducing a SciPy KDTree cull and Numba-accelerated spherical winding‐number tests, and refactors legacy implementations.
- Adds
uxarray/grid/point_in_face.pywith Numba functions for single and batched point‐in‐face queries using spherical winding numbers. - Updates
Grid.get_faces_containing_pointto a keyword‐only API that delegates to the new batched query path. - Replaces legacy geometry routines in
uxarray/grid/geometry.pywith parallelized Euclidean radius and winding‐number implementations and removes deprecated functions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uxarray/grid/point_in_face.py | New Numba routines for fast face‐in‐point queries, batched support |
| uxarray/grid/grid.py | Refactored get_faces_containing_point signature and call path |
| uxarray/grid/geometry.py | Replaced old point‐in‐face and radius logic with parallel versions |
| test/test_point_in_face.py | Added tests for new point‐in‐face behavior and return_counts |
| test/test_grid.py | Removed outdated face containment tests |
| test/test_geometry.py | Updated tests to call _face_contains_point and normalize coords |
Comments suppressed due to low confidence (3)
uxarray/grid/geometry.py:1082
- The code uses
math.sqrtbutmathis not imported in this module. Addimport mathat the top.
return math.sqrt(global_max2)
uxarray/grid/grid.py:2610
- The function
_lonlat_rad_to_xyzis not imported in this file. Addfrom uxarray.grid.coordinates import _lonlat_rad_to_xyzto the imports.
point_xyz = _lonlat_rad_to_xyz(lon, lat)
uxarray/grid/point_in_face.py:216
- [nitpick] Casting lists to
int64may cause a later implicit conversion toINT_DTYPE(e.g., int32) in the Numba call. Cast directly toINT_DTYPEfor consistency and to avoid extra copies.
flat_cands = np.concatenate([np.array(lst, dtype=np.int64) for lst in cand_lists])
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the point‐in‐face computation by introducing a SciPy KDTree for candidate culling and a new spherical winding‑number approach accelerated with Numba.
- Implements new functions (_face_contains_point, _batch_point_in_face, etc.) for efficient point‐in‐face querying.
- Updates Grid.get_faces_containing_point to support batched queries and both Cartesian and spherical inputs.
- Refactors related utility and test functions to align with the new implementation.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uxarray/grid/point_in_face.py | Introduces optimized functions with KDTree culling and winding tests. |
| uxarray/grid/grid.py | Updates the API and docstring for get_faces_containing_point. |
| uxarray/grid/geometry.py | Revises max face radius calculation with parallel Numba implementation. |
| uxarray/grid/coordinates.py | Adds _prepare_points_for_kdtree and updates error handling. |
| test/* | Updates and adds tests to cover the new behavior and edge cases. |
Comments suppressed due to low confidence (2)
uxarray/grid/coordinates.py:873
- The error message contains a typo ('One one can be provided'). Please change it to 'Only one can be provided at a time' for clarity.
Both Cartesian (xyz) and Spherical (lonlat) coordinates were provided. One one can be provided at a time.
uxarray/grid/geometry.py:1082
- The math module is used here but it is not imported. Add 'import math' at the top of the module to ensure math.sqrt is available.
return math.sqrt(global_max2)
Overview
Grid.get_faces_containing_point()