Simplify DataArray dims - derive from geometry #893
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add Geometry0D class for time-series-only data (dfs0) - Add reduce() methods to geometries for spatial aggregations - Make dims a derived property from geometry.spatial_dims - Rename default_dims to spatial_dims across all geometries - Add deprecation warnings for v4.0 migration: - dims parameter usage - squeeze() method - Non-equidistant isel() operations - GeometryUndefined instantiation - Update spectral geometries with proper spatial_dims BREAKING CHANGES (with deprecation warnings): - dims parameter is deprecated, dimensions derived from geometry - squeeze() method deprecated, use isel() instead - Non-equidistant grid selections will error in v4.0 - GeometryUndefined deprecated, use Geometry0D
Replace FutureWarning + GeometryUndefined pattern with immediate ValueError. This fails fast when users attempt non-equidistant selections that cannot be written to DFS format, avoiding late surprises in their workflows. - Grid2D.isel(), Grid3D.isel(), Grid3D._index_to_Grid3D(), Grid3D._geometry_for_layers() now raise ValueError immediately - Remove deprecation warning from GeometryUndefined.__init__ (class kept for backwards compatibility but no longer instantiated) - Update tests to expect ValueError instead of FutureWarning
- Add reduce() method to _Geometry base class - Rename spatial_dims to dims across all geometry classes - Remove axis_name from Grid1D and axis_names from Grid2D - Grid1D.isel returns Geometry0D instead of GeometryUndefined - Update tests to use Geometry0D and expect "x" dimension from Grid1D - Remove deprecated squeeze() and dims parameter usage from tests
Performance improvements and type corrections: - Use @cached_property for dims and _has_time_axis in DataArray - Eliminates redundant computation in hot paths (isel, aggregate, etc.) - dims accessed 36+ times across codebase, now computed only once - Fix return types for geometry.reduce() methods (Grid2D, Grid3D) - Update base _Geometry.reduce() to return _Geometry for covariance
- Remove dims= from squeeze(), isel(), aggregate(), and quantile() methods - Dims are now always derived from geometry via @cached_property - Update docstring to mark dims parameter as deprecated - All 755 tests pass - dims derivation from geometry works correctly
- Remove dfsu, dfs0, dfs2, and nc files from notebooks/ - Add gitignore rules to prevent committing notebook outputs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Geometry0Dclass for time-series-only datadimsa derived property fromgeometry.spatial_dimsreduce()methods to geometries for spatial aggregations@cached_propertyfordimsand_has_time_axisfor performance optimizationBreaking changes
dimsparameter deprecated (FutureWarning) - dimensions now derived from geometrysqueeze()method deprecated (FutureWarning) - useisel()insteadValueErrorimmediately (DFS format compliance)GeometryUndefineddeprecated - useGeometry0DHistorical Context: The
dimsParameterThe
dimsparameter was introduced in January 2022 (commit1133864f) with a guessing heuristic:The original comment "This is not very robust, but is probably a reasonable guess" acknowledged this was always a heuristic, not a proper solution.
The Core Problem
The
dimsparameter was copied from xarray's API (for xarray integration), but created a false impression:("lat", "lon", "depth", "ensemble_member")time, xtime, y, xtime, z, y, xtime, elementAllowing users to pass custom
dimscreated an illusion of possibilities not supported by the DFS format. Custom dims wouldn't persist through write/read cycles.The Solution
This PR completes the work started in March 2024 to make dims truly derivable:
aa1afc15): Started delegating dims to geometry classes04e1e2d9,a7931470): Made dims a derived property, removed parameterDimensions are now always derived from geometry, eliminating the illusion that they could be arbitrary and ensuring they correctly represent the actual DFS file structure.