fix: rename SCHISM geographic coords in _rename_coords for uxarray ≥ 2025.8.0#6
Merged
Conversation
…2025.08 compatibility
uxarray's _read_ugrid gained an outer existence guard between v2025.06.0 and
v2025.08.0: face/edge coordinate renaming via the face_coordinates UGRID
attribute now only fires if variables literally named "face_lon"/"face_lat"
already exist in the dataset. Since suxarray stores pre-transformed coordinates
as SCHISM_hgrid_face_lon/lat (not face_lon/lat), the rename was silently skipped,
leaving face_lon absent from _ds.
Without face_lon, uxarray's _populate_face_centroids falls back to the Cartesian
branch. It finds face_x in _ds (UTM easting, renamed by _rename_coords) and grabs
face_y (UTM northing) and face_z (SCHISM bottom-layer depth in negative meters,
stored by _assign_z_coords) as if they were unit-sphere Cartesian coordinates.
It then calls:
lat = arcsin(face_z) # arcsin only defined for z ∈ [-1, 1]
arctan2 used for lon never produces NaN, so face_lon is unaffected. arcsin on
depth values outside [-1, 1] produces NaN, so only face_lat is corrupted. On the
Bay-Delta test domain, about ~4% (~21K of
538K) faces get corrupted.
Fix: add explicit SCHISM_hgrid_{node,face,edge}_lon/lat → {node,face,edge}_lon/lat
renaming in _rename_coords with if-guards. Under uxarray 2025.06, _read_ugrid
already consumed these names so the guards are false (no-op). Under 2025.08+,
the rename ensures face_lon is present in _ds before uxarray's lazy centroid
property fires, taking _populate_face_centroids down the correct else-branch
(converts existing face_lon/lat to face_x/y/z) with no recomputation.
kjnam
approved these changes
Jul 6, 2026
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
2 participants
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.
uxarray's _read_ugrid gained an outer existence guard between v2025.06.0 and v2025.08.0: face/edge coordinate renaming via the face_coordinates UGRID attribute now only fires if variables literally named "face_lon"/"face_lat" already exist in the dataset. Since suxarray stores pre-transformed coordinates as SCHISM_hgrid_face_lon/lat (not face_lon/lat), the rename was silently skipped, leaving face_lon absent from _ds.
Without face_lon, uxarray's _populate_face_centroids falls back to the Cartesian branch. It finds face_x in _ds (UTM easting, renamed by _rename_coords) and grabs face_y (UTM northing) and face_z (SCHISM bottom-layer depth in negative meters, stored by _assign_z_coords) as if they were unit-sphere Cartesian coordinates. It then calls:
arctan2 used for lon never produces NaN, so face_lon is unaffected. arcsin on depth values outside [-1, 1] produces NaN, so only face_lat is corrupted. On the Bay-Delta test domain, about ~4% (~21K of 538K) faces get corrupted.
Fix: add explicit SCHISM_hgrid_{node,face,edge}_lon/lat → {node,face,edge}_lon/lat renaming in _rename_coords with if-guards. Under uxarray 2025.06, _read_ugrid already consumed these names so the guards are false (no-op). Under 2025.08+, the rename ensures face_lon is present in _ds before uxarray's lazy centroid property fires, taking _populate_face_centroids down the correct else-branch (converts existing face_lon/lat to face_x/y/z) with no recomputation.
Additionally, the uxarray dependency version in pyproj.toml was brought up to 2025.12.0