-
Notifications
You must be signed in to change notification settings - Fork 38
Refactor & Optimize Remapping (NN & IDW) #1244
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
A couple thing to note here that are relevant to #1016 I've unified howe we do our remapping and dramatically streamlined it so that we don't need separate |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
So, we should hold off the bilinear remapping PR and Mark this main first? |
It shouldn't be much of a delay. I'm planning to move forward with this PR before the hackathon next week. |
Ok, I was hoping we get bilinear before the hackathon |
I'll have this ready today. Are the recent changes in for Bilinear? Getting Bilinear merged for the hackathon may not be practical if it can't be run on the grids we'll be using, since most grids will be very high resolution. |
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.
Pull Request Overview
This PR refactors the remapping functionality and unifies the remapping accessors for both UxDataset and UxDataArray while optimizing nearest‐neighbor and inverse‐distance‐weighted routines. Key changes include replacing deprecated parsing functions with new implementations in the remap module, consolidating accessor classes into a unified RemapAccessor, and enhancing grid nearest neighbor support via a KDTree cache.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
uxarray/remap/utils.py | Refactored remap helper functions and removed deprecated parsing code. |
uxarray/remap/nearest_neighbor.py | Renamed and restructured nearest neighbor functions with clearer APIs. |
uxarray/remap/inverse_distance_weighted.py | Revised IDW remap implementation and introduced weight calculation helper. |
uxarray/remap/accessor.py | Unified remapping accessors into one RemapAccessor interface. |
uxarray/grid/grid.py | Added KDTree caching for grid queries and new get_kdtree method. |
uxarray/core/dataset.py & dataarray.py | Updated remap accessor usage and revised isel signature for grid-aware slicing. |
uxarray/grid/coordinates.py | Minor adjustments in coordinate population and removal of unnecessary warning. |
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.
Pull Request Overview
This PR refactors and optimizes the remapping functionality by unifying the remapping accessor for both UxDataset and UxDataArray, streamlining the underlying nearest‐neighbor and inverse‐distance–weighted implementations, and defaulting to a Cartesian KDTree (from scipy.spatial) for all remapping operations.
- Unified remapping functions and removed legacy dataset/dataarray accessors.
- Updated function signatures and docstrings in remapping modules for improved clarity and error handling.
- Incorporated KDTree caching in the grid module and updated accessor references in core dataset and dataarray modules.
- Updated CI dependencies by adding "cmocean".
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
uxarray/remap/utils.py | Added several helper functions (_assert_dimension, _construct_remapped_ds, _to_dataset, _get_remap_dims, _prepare_points) and improved docstrings and error handling. |
uxarray/remap/nearest_neighbor.py | Renamed and refactored nearest neighbor remapping functions; updated parameter names and documentation for clarity. |
uxarray/remap/inverse_distance_weighted.py | Refactored inverse-distance–weighted remapping, extracting a dedicated weight computation helper (_idw_weights) and updating function interface. |
uxarray/remap/accessor.py | Introduced a unified RemapAccessor providing remapping methods on both UxDataset and UxDataArray, with updated supported parameter values. |
uxarray/remap/init.py | Adjusted exports to reflect the refactored remapping functions. |
uxarray/grid/grid.py | Added KDTree caching with a new get_kdtree method using scipy.spatial.KDTree. |
uxarray/grid/coordinates.py | Minor changes to coordinate-related warnings (now commented out). |
uxarray/core/dataset.py | Updated the remap accessor to use the new unified RemapAccessor. |
uxarray/core/dataarray.py | Updated the remap accessor to use the new unified RemapAccessor and expanded the isel signature. |
ci/docs.yml | Added the "cmocean" dependency. |
The changes are in for bilinear. It should work, but is not optimized yet, especially since I had to redo the weights calculation. With the updated tree structures that might help, but until the point contains function is more performative, bilinear will be slower, as that is the main bottleneck according to the benchmarks. |
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.
Pull Request Overview
This PR refactors and optimizes the remapping functionality by unifying the remapping accessor for both UxDataset and UxDataArray, while streamlining the underlying nearest neighbor and inverse‐distance–weighted implementations. In addition, it removes the old dedicated remap accessors, updates related documentation and CI dependencies, and adds caching for KDTree construction in the Grid module.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
uxarray/remap/***.py | Refactored remapping functions and renamed internal APIs (e.g. _nearest_neighbor_query, _inverse_distance_weighted_remap). |
uxarray/remap/accessor.py | Introduced a unified RemapAccessor replacing the removed dataset/dataarray accessor modules. |
uxarray/core/dataset.py and uxarray/core/dataarray.py | Updated to use the new RemapAccessor. |
uxarray/grid/grid.py | Added a new method (_get_scipy_kd_tree) and caching for KDTree instances. |
ci/docs.yml | Added dependency on “cmocean” to the documentation build environment. |
Comments suppressed due to low confidence (2)
uxarray/remap/accessor.py:38
- The default value for the 'remap_to' parameter is set to 'faces', but the underlying remapping functions validate dimensions against values like 'nodes', 'edge centers', or 'face centers'. Consider changing the default to 'face centers' to ensure consistency in the API.
def nearest_neighbor(self, destination_grid: Grid, remap_to: str = "faces", **kwargs) -> UxDataArray | UxDataset:
uxarray/remap/accessor.py:60
- The default value for 'remap_to' here is also set to 'faces', which may not match the expected keys in the remapping routines. Updating this default to 'face centers' would improve clarity and consistency across the API.
def inverse_distance_weighted(self, destination_grid: Grid, remap_to: str = "faces", power=2, k=8, **kwargs) -> UxDataArray | UxDataset:
Overview
scipy.spatial.KDTree
for all remapping operationsUxDataset
orUxDataArray
UxDataset
andUxDataArray
isel()
which didn't allow a dictionary to be passed in for grid dimensionsBenchmarks
M1 Macbook Pro (32GB)
Derecho CPU Node