Move projection-based axis labels and aspect ratio onto geometry - #991
Open
ecomodeller wants to merge 2 commits into
Open
Move projection-based axis labels and aspect ratio onto geometry#991ecomodeller wants to merge 2 commits into
ecomodeller wants to merge 2 commits into
Conversation
The axis labels for spatial plots (Easting/Northing, Longitude/Latitude, etc.) and the plot aspect ratio were decided by if/elif branches inside the plotting code, duplicated across _FM_plot, _grid_geometry and _data_plot. The discriminators (is_geo, is_local_coordinates, is_spectral, _is_rotated) are all geometry properties, so the decision belongs on the geometry. Geometries now expose `_axis_labels` and `_plot_aspect`; every plotter just reads them. This removes the duplication and the branching from the plotters. NON-UTM grids now label "x [m]" / "y [m]" consistently (previously "[m]" in the grid path), matching the FM convention.
There was a problem hiding this comment.
Pull request overview
This PR refactors spatial plotting so axis labels and plot aspect ratio are derived from geometry properties instead of duplicated if/elif logic inside multiple plotters, aiming to centralize and standardize projection-dependent plotting behavior.
Changes:
- Added
_Geometry._axis_labelsand a shared_geographic_aspect()helper to compute projection-based axis labeling and geographic aspect correction. - Updated Grid2D/Flexible Mesh geometries to expose
_plot_aspectand (where needed) override_axis_labels, and updated plotters to consume these properties. - Removed now-duplicated projection/aspect branching helpers from the flexible-mesh plotting code.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mikeio/spatial/_grid_geometry.py | Uses geometry-provided labels/aspect in plotting; adds Grid2D _axis_labels override and _plot_aspect property. |
| src/mikeio/spatial/_geometry.py | Introduces _geographic_aspect() and base _Geometry._axis_labels to centralize projection-based axis labels. |
| src/mikeio/spatial/_FM_plot.py | Removes projection branching helpers and reads _axis_labels/_plot_aspect from FM geometry directly. |
| src/mikeio/spatial/_FM_geometry.py | Moves FM aspect calculation onto geometry via _plot_aspect and updates plotting methods to use geometry labels/aspect. |
| src/mikeio/dataset/_data_plot.py | Removes duplicated projection/aspect branching for Grid2D plotting and consumes geometry _axis_labels/_plot_aspect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+505
to
+511
| @property | ||
| def _plot_aspect(self) -> Literal["equal"] | float | None: | ||
| if self.is_spectral or self._is_rotated or self.is_local_coordinates: | ||
| return None | ||
| if self.is_geo: | ||
| return _geographic_aspect(self.y) | ||
| return "equal" |
Comment on lines
+92
to
+96
| if self.is_geo: | ||
| return "Longitude [degrees]", "Latitude [degrees]" | ||
| if self.is_local_coordinates: # NON-UTM | ||
| return "x [m]", "y [m]" | ||
| return "Easting [m]", "Northing [m]" |
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
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.
Axis labels for spatial plots and the plot aspect ratio were decided by
if/elifbranches living inside the plotting code, duplicated across_FM_plot,_grid_geometryand_data_plot(the latter two byte-for-byte identical). The branches keyed entirely off geometry properties —is_geo,is_local_coordinates,is_spectral,_is_rotated— so the decision belongs on the geometry, not the plotter.Geometries now expose two properties,
_axis_labelsand_plot_aspect, and every plotter just reads them. This removes the duplication, takes the branching out of the plotting classes, and makes the labelling consistent across the structured-grid and flexible-mesh plot paths.One intentional behaviour change: NON-UTM (local-coordinate) grids now label
x [m]/y [m], where the structured-grid path previously used[m]/[m]. This unifies on the convention the flexible-mesh path already used.