Add optional YAC remapping backend with NNN/conservative support and CI coverage#1440
Add optional YAC remapping backend with NNN/conservative support and CI coverage#1440
Conversation
| tgt_points = self._tgt_grid.def_points(np.deg2rad(tgt_lon), np.deg2rad(tgt_lat)) | ||
| return src_points, tgt_points | ||
|
|
||
| def remap(self, values: np.ndarray) -> np.ndarray: |
There was a problem hiding this comment.
Can you be more specific here about what's "remap"? Is it conservative or non-conservative? You can reflect it either in function name or comment
|
In your application the setup of YAC should get much easier (e.g. no setting of calendar or start/end time required) by using the (unfortunately basically undocumented) Python bindings for the yac_core library. Here is some basic documentation: This is an example using this interface: You can configure YAC to only build the yac_core library (which does not depend on libfyaml). In addition libnetcdf is also optional. |
There was a problem hiding this comment.
Pull request overview
Adds an optional YAC-backed remapping backend to UXarray so users can route remap operations through YAC’s interpolation machinery when requested, while keeping existing UXarray behavior as the default.
Changes:
- Introduces a new YAC backend implementation for NNN (nearest-neighbor) and conservative remapping.
- Extends the
.remapaccessor API to acceptbackend="yac"plusyac_method/yac_options. - Adds YAC-only tests and a dedicated GitHub Actions workflow to build YAC/YAXT from source and run those tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
uxarray/remap/yac.py |
New YAC backend implementation and import/loading logic. |
uxarray/remap/accessor.py |
Adds backend/method/options parameters and routes to YAC backend when requested. |
test/test_remap_yac.py |
Adds YAC integration/parity tests (skipped when YAC unavailable). |
.github/workflows/yac-optional.yml |
New optional CI job that builds YAXT+YAC and runs YAC-specific tests. |
.github/workflows/ci.yml |
Sets MPLBACKEND=Agg in CI environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remapper = remappers[src_dim] | ||
| out_flat = np.empty((flat_src.shape[0], remapper._tgt_size), dtype=np.float64) | ||
| for idx in range(flat_src.shape[0]): | ||
| out_flat[idx] = remapper.remap(flat_src[idx]) | ||
|
|
There was a problem hiding this comment.
The remap currently loops in Python over flat_src.shape[0] and calls into YAC once per slice. For multi-dimensional data this can be a major bottleneck. If the YAC interpolation supports batched collections, consider requesting collection_size=flat_src.shape[0] and calling the interpolation once on the full (collection_size, src_size) array (or otherwise vectorizing) to avoid per-slice overhead.
| def _yac_remap(source, destination_grid, remap_to: str, yac_method: str, yac_kwargs): | ||
| _assert_dimension(remap_to) | ||
| destination_dim = LABEL_TO_COORD[remap_to] | ||
| options = _normalize_yac_method(yac_method) | ||
| options.kwargs.update(yac_kwargs or {}) | ||
| ds, is_da, name = _to_dataset(source) | ||
| dims_to_remap = _get_remap_dims(ds) | ||
| remappers: dict[str, _YacRemapper] = {} | ||
| remapped_vars = {} | ||
|
|
||
| for src_dim in dims_to_remap: | ||
| remappers[src_dim] = _YacRemapper( | ||
| ds.uxgrid, | ||
| destination_grid, | ||
| src_dim, | ||
| destination_dim, | ||
| options.method, | ||
| options.kwargs, | ||
| ) |
There was a problem hiding this comment.
PR description says conservative remap is supported for face-centered data only, but _yac_remap/_YacRemapper don’t enforce that. As written, users can request yac_method='conservative' while remapping node/edge data, which is likely to either error deep inside YAC or produce unexpected semantics. Add an explicit validation that conservative is only used when both source and destination locations are face/cell-based (e.g. src_dim=='n_face' and destination_dim=='n_face').
| out = uxds["latCell"].remap.nearest_neighbor( | ||
| destination_grid=dest, | ||
| remap_to="faces", | ||
| backend="yac", | ||
| yac_method="conservative", |
There was a problem hiding this comment.
This test exercises yac_method='conservative' through the nearest_neighbor(...) API, which is semantically confusing (it’s not nearest-neighbor). Once the public API is clarified (e.g., a dedicated conservative(...) method or a generic .remap(...) entrypoint), update this test to use the method that matches the behavior being tested.
| out = uxds["latCell"].remap.nearest_neighbor( | |
| destination_grid=dest, | |
| remap_to="faces", | |
| backend="yac", | |
| yac_method="conservative", | |
| out = uxds["latCell"].remap.conservative( | |
| destination_grid=dest, | |
| remap_to="faces", | |
| backend="yac", |
| def __call__( | ||
| self, | ||
| *args, | ||
| backend: str = "uxarray", | ||
| yac_method: str | None = None, | ||
| yac_options: dict | None = None, | ||
| **kwargs, | ||
| ) -> UxDataArray | UxDataset: | ||
| """ | ||
| Shortcut for nearest-neighbor remapping. | ||
|
|
||
| Calling `.remap(...)` with no explicit method will invoke | ||
| `nearest_neighbor(...)`. | ||
| """ | ||
| return self.nearest_neighbor(*args, **kwargs) | ||
| return self.nearest_neighbor( | ||
| *args, | ||
| backend=backend, | ||
| yac_method=yac_method, | ||
| yac_options=yac_options, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
RemapAccessor.__call__ forwards yac_method=None into nearest_neighbor(...), which overrides that method’s default of 'nnn' and will cause .remap(..., backend='yac') to raise ValueError unexpectedly. Consider defaulting yac_method to 'nnn' here when backend=='yac', or only passing yac_method through when it’s not None.
| if backend == "yac": | ||
| from uxarray.remap.yac import _yac_remap | ||
|
|
||
| yac_kwargs = yac_options or {} | ||
| return _yac_remap( | ||
| self.ux_obj, destination_grid, remap_to, yac_method, yac_kwargs | ||
| ) | ||
| return _nearest_neighbor_remap(self.ux_obj, destination_grid, remap_to) |
There was a problem hiding this comment.
backend is only checked for the special-case value 'yac'; any other unexpected value silently falls back to the UXarray implementation. To avoid typos changing behavior without warning, validate backend (e.g., allow only 'uxarray'/'yac' and raise ValueError otherwise).
| from uxarray.remap.yac import _yac_remap | ||
|
|
||
| yac_kwargs = yac_options or {} | ||
| return _yac_remap( | ||
| self.ux_obj, destination_grid, remap_to, yac_method, yac_kwargs |
There was a problem hiding this comment.
When backend=='yac', inverse_distance_weighted(...) routes to _yac_remap(...), but _yac_remap only supports 'nnn' / 'conservative' and will not perform IDW. This makes inverse_distance_weighted(backend='yac') return results that don’t match the method name/contract. Consider raising NotImplementedError/ValueError for backend='yac' here until a real YAC-IDW implementation exists (or map explicitly to the equivalent YAC method if that’s intended).
| from uxarray.remap.yac import _yac_remap | |
| yac_kwargs = yac_options or {} | |
| return _yac_remap( | |
| self.ux_obj, destination_grid, remap_to, yac_method, yac_kwargs | |
| raise NotImplementedError( | |
| "inverse_distance_weighted with backend='yac' is not implemented. " | |
| "The YAC backend currently supports only 'nnn' and 'conservative' " | |
| "methods and will not perform inverse-distance-weighted remapping. " | |
| "Use backend='uxarray' for IDW, or choose a different remapping " | |
| "method that is supported by YAC." |
| if backend == "yac": | ||
| from uxarray.remap.yac import _yac_remap | ||
|
|
||
| yac_kwargs = yac_options or {} | ||
| return _yac_remap( | ||
| self.ux_obj, destination_grid, remap_to, yac_method, yac_kwargs | ||
| ) |
There was a problem hiding this comment.
When backend=='yac', bilinear(...) routes to _yac_remap(...), but _yac_remap only supports 'nnn' / 'conservative' and will not perform bilinear interpolation. This means bilinear(backend='yac') can silently do a different method than requested. Consider rejecting backend='yac' for bilinear until a true YAC-bilinear path is implemented (or wire it up explicitly).
| remapped_vars[var_name] = da_out | ||
|
|
||
| ds_remapped = _construct_remapped_ds( | ||
| source, remapped_vars, destination_grid, destination_dim |
There was a problem hiding this comment.
_construct_remapped_ds(...) expects remap_to to be one of 'nodes'/'edges'/'faces' (used by SpatialCoordsRemapper), but _yac_remap passes destination_dim (e.g. 'n_node'). This will raise at runtime when constructing output coords. Pass the original remap_to label into _construct_remapped_ds instead of destination_dim.
| source, remapped_vars, destination_grid, destination_dim | |
| source, remapped_vars, destination_grid, remap_to |
Closes #1441, Close #897