Raise ValueError in _get_ordered_vertices() with "mixed" order
#595
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 85.78% 86.75% +0.96%
==========================================
Files 13 15 +2
Lines 2364 3155 +791
Branches 183 302 +119
==========================================
+ Hits 2028 2737 +709
- Misses 303 367 +64
- Partials 33 51 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cf_xarray/helpers.py
Outdated
| elif order == "descending": | ||
| endpoints = np.maximum(bounds[..., :, 0], bounds[..., :, 1]) | ||
| last_endpoint = np.minimum(bounds[..., -1, 0], bounds[..., -1, 1]) | ||
| elif order == "mixed": |
There was a problem hiding this comment.
Even if this is a private function, I would avoid having code that can result in an UnboundLocalError.
I suggest either replacing it with a bare else, or adding something like:
else:
raise NotImplementedError(f"{order = }")There was a problem hiding this comment.
The new code raises a ValueError with order=mixed, so it shouldn't result in UnboundLocalError. However, I updated this conditional to else as a fall-back for any order value (even if it should only be ascending, descending, or mixed).
tomvothecoder
force-pushed
the
bug/594-mixed-coords
branch
from
9c287cf to
1a5eb22
Compare
for more information, see https://pre-commit.ci
| else: | ||
| raise NotImplementedError( | ||
| f"Cannot determine vertices for non-monotonic bounds with {order} core " |
There was a problem hiding this comment.
A user reported an issue in #594 where
_get_ordered_verticesfails to set theendpointsvariable when the order of core dims are"mixed". In their case, the longitude coordinates are circular (0–360), and the 1-D core dimension isn’t strictly monotonic because it crosses the seam at 0°.We should raise a
ValueErrorfor"mixed"cases, rather than silently fail (or normalize).Long-term solution
Longer term, somebody can open a PR to consider opt-in support for circular axes, e.g.
bounds_to_vertices(..., circular_period=360.0, start=None). Withcircular_periodset, cf-xarray could detect circular monotonicity, rotate away from the seam, and proceed safely.