-
Notifications
You must be signed in to change notification settings - Fork 22
Improve global stereographic meshing with configurable k0 and CRS support #89
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
base: master
Are you sure you want to change the base?
Conversation
…g, refactor region transforms to use it, and fully propagate scale_factor (k0) from Shoreline/Domain into generate_mesh and stereographic distortion routines, fixing syntax issues in mesh_generator.
Add configurable stereographic scale_factor (k0) plumbed through Shoreline → Domain → meshing and gradient enforcement, with Cartopy-backed and analytic fallbacks. Make global and multiscale CRS handling more permissive and CRS-aware, including mixed CRS validation and global bbox detection. Improve polar gradient limiting using stereographic distortion correction and add/adjust tests for stereo, k0, and mixed-CRS multiscale cases. Expand documentation (module docstrings and README) to describe stereographic workflow, k0 semantics, and Cartopy usage.
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 enhances OceanMesh's global stereographic meshing capabilities by introducing a configurable stereographic scale factor (k₀), improving CRS handling for global and multiscale meshes, and refining polar gradient enforcement with optional cartopy integration.
Key Changes
- Added
scale_factorparameter throughout the meshing pipeline (Shoreline → Domain → mesh generation) to enable configurable stereographic distortion control - Introduced optional cartopy-backed projection support with fallback to legacy analytic formulas for improved accuracy and CRS flexibility
- Relaxed CRS validation rules to support alternative geographic and projected coordinate systems while maintaining compatibility warnings
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_global_stereo.py | Added comprehensive tests for custom k₀ values, alternative CRS support, and polar gradient distortion correction |
| tests/test_global_regional_multiscale.py | Extended tests to validate mixed CRS combinations in multiscale meshing scenarios |
| setup.cfg | Introduced global extras_require for optional cartopy dependency |
| oceanmesh/signed_distance_function.py | Propagated scale_factor through Domain classes and multiscale SDF operations |
| oceanmesh/region.py | Enhanced CRS validation, added stereographic helpers with cartopy integration, and improved global bbox detection |
| oceanmesh/projections.py | New module providing StereoProjection class for cartopy-backed north-polar stereographic transformations |
| oceanmesh/mesh_generator.py | Integrated scale_factor into meshing workflow and updated CRS validation for global/regional combinations |
| oceanmesh/grid.py | Added documentation for stereographic coordinate transformation in plotting |
| oceanmesh/geodata.py | Extended Shoreline class with scale_factor support and CRS validation for global domains |
| oceanmesh/edgefx.py | Refactored polar gradient enforcement with distortion-aware corrections using configurable k₀ |
| oceanmesh/init.py | Exposed CARTOPY_AVAILABLE flag for runtime cartopy detection |
| README.md | Added comprehensive documentation for stereographic projections, scale factor configuration, and cartopy integration |
Comments suppressed due to low confidence (1)
oceanmesh/projections.py:29
- Import of 'ccrs' is not used.
import cartopy.crs as ccrs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scale_factor_vals = [getattr(d, "scale_factor", 1.0) for d in domains] | ||
| scale_factor = max(scale_factor_vals) if scale_factor_vals else 1.0 |
Copilot
AI
Dec 8, 2025
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.
Taking the max() of scale_factor values when combining domains may not be semantically correct. When merging domains with different scale factors (e.g., Union/Intersection/Difference operations), the max() heuristic doesn't preserve the physical meaning of the scale factor for each subdomain. Consider either requiring all domains to have the same scale_factor or documenting why max() is the appropriate choice, or raising a warning when domains have different scale factors.
| scale_factor_vals = [getattr(d, "scale_factor", 1.0) for d in domains] | ||
| scale_factor = max(scale_factor_vals) if scale_factor_vals else 1.0 |
Copilot
AI
Dec 8, 2025
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.
Taking the max() of scale_factor values when combining domains may not be semantically correct. When merging domains with different scale factors (e.g., Union/Intersection/Difference operations), the max() heuristic doesn't preserve the physical meaning of the scale factor for each subdomain. Consider either requiring all domains to have the same scale_factor or documenting why max() is the appropriate choice, or raising a warning when domains have different scale factors.
|
|
||
| from . import _version | ||
| try: # Optional global-stereo helpers (import may fail generically) | ||
| from .projections import CARTOPY_AVAILABLE |
Copilot
AI
Dec 8, 2025
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.
StereoProjection is added to __all__ but never imported into the module namespace. This will cause an AttributeError when users try to access oceanmesh.StereoProjection. Add from .projections import StereoProjection before extending __all__.
| from .projections import CARTOPY_AVAILABLE | |
| from .projections import CARTOPY_AVAILABLE, StereoProjection |
|
|
||
| # Resolution in stereographic metres, based on underlying lon/lat step. | ||
| dx_deg = grid.dx | ||
| dx_stereo = dx_deg * np.pi / 180.0 * 6_371_000.0 |
Copilot
AI
Dec 8, 2025
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.
The Earth radius value 6_371_000.0 is hardcoded here and also appears on line 170. Consider defining this as a module-level or function-level constant (e.g., EARTH_RADIUS_M = 6_371_000.0) to avoid magic numbers and ensure consistency. This would improve maintainability if the value needs to be adjusted or if different ellipsoid models are used in the future.
| # Choose a reasonable physical radius (in metres) around the | ||
| # north pole within which to regularise the gradient. Here we | ||
| # use ~2000 km. | ||
| r_max = 2_000_000.0 |
Copilot
AI
Dec 8, 2025
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.
The polar radius value 2_000_000.0 (~2000 km) is hardcoded. Consider defining this as a configurable parameter or a named constant (e.g., POLAR_REGULARIZATION_RADIUS_M = 2_000_000.0) to improve maintainability and allow users to adjust the polar regularization extent if needed for different use cases.
| if CARTOPY_AVAILABLE: | ||
| check_cartopy_available() | ||
| else: | ||
| logger.warning( | ||
| "Shoreline: cartopy is not available; falling back to legacy stereographic " | ||
| "formulas for stereo=True workflows. Consider installing cartopy for more " | ||
| "accurate global projections." | ||
| ) |
Copilot
AI
Dec 8, 2025
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.
This logic is contradictory: when CARTOPY_AVAILABLE is True, it calls check_cartopy_available() which will succeed silently. When CARTOPY_AVAILABLE is False, it logs a warning but continues. The condition should be inverted: only call check_cartopy_available() (which raises an error) when cartopy is NOT available, or remove the check entirely since the fallback is intentional. As written, the check has no effect.
| ok, errors = om.mesh_generator._validate_multiscale_domains( # type: ignore[attr-defined] | ||
| [sdf_global, sdf_regional], [edge_global, edge_regional] | ||
| ) |
Copilot
AI
Dec 8, 2025
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.
This test accesses a private function _validate_multiscale_domains using # type: ignore[attr-defined]. While testing internal validation logic can be valuable, consider whether this functionality should be exposed through a public API or whether the validation is adequately covered by integration tests. Direct testing of private functions can create maintenance burden if the internal implementation changes.
| epsg = None | ||
|
|
Copilot
AI
Dec 8, 2025
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.
Variable epsg is not used.
| epsg = None |
| o = CRS.from_user_input(outer_crs) | ||
| i = CRS.from_user_input(inner_crs) | ||
| except Exception: | ||
| o = i = None |
Copilot
AI
Dec 8, 2025
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.
Variable i is not used.
| o = i = None | |
| o = None |
| proj = _get_stereo_projection(scale_factor=k0) | ||
| if proj is not None: | ||
| return proj.get_scale_factor(lat) * np.pi / 180.0 | ||
| except Exception: |
Copilot
AI
Dec 8, 2025
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.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # If import or projection fails, fall back to analytic formula below. |
Summary
@stefanzieger @tomsail
This PR improves oceanmesh's global stereographic meshing by introducing a configurable stereographic scale factor, making CRS handling more flexible for global and multiscale setups, and tightening polar gradient enforcement.
Details