Skip to content

Conversation

larsbuntemeyer
Copy link
Contributor

@larsbuntemeyer larsbuntemeyer commented Aug 15, 2025

closes #581

Improves the handling of datetime timedelta objects in core dimension ordering and bounds monotonicity checks within the cf_xarray/helpers.py module. The changes ensure that comparisons involving timedeltas are correctly performed using their total seconds.

Enhancements for datetime timedelta support:

  • Updated _get_core_dim_orders to detect and process datetime.timedelta differences using the total_seconds method for safe numeric comparison.
  • Refactored _is_bounds_monotonic to check for datetime.timedelta in bounds differences and convert them to seconds before monotonicity evaluation, instead of always casting to float64.
  • Add tests.

Copy link

codecov bot commented Aug 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.64%. Comparing base (a9cebee) to head (3171796).
⚠️ Report is 67 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #582      +/-   ##
==========================================
+ Coverage   85.78%   86.64%   +0.86%     
==========================================
  Files          13       15       +2     
  Lines        2364     3033     +669     
  Branches      183      296     +113     
==========================================
+ Hits         2028     2628     +600     
- Misses        303      353      +50     
- Partials       33       52      +19     
Flag Coverage Δ
mypy 44.26% <11.11%> (+5.73%) ⬆️
unittests 93.52% <100.00%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the fix, and sorry for the breaking change. I didn't think of time dtypes when reviewing this.

Out of curiosity, what workflow are you doing that requires a bounds_to_vertices on the time bounds ?

I can approve and release the patch when a test is added.

@larsbuntemeyer
Copy link
Contributor Author

larsbuntemeyer commented Aug 15, 2025

Out of curiosity, what workflow are you doing that requires a bounds_to_vertices on the time bounds ?

I do lots of cmorization! Sometimes need it for the cmor.axis API, but now as you asked, i looked closer into the cell_bounds keyword and it seems it also accepts the 2d version with (2, n) dims.

Yes, i need it for cmorization

In the case of a 1-d array, the size is one more than the size of coord_vals and the cells must be contiguous.

@larsbuntemeyer larsbuntemeyer marked this pull request as ready for review August 18, 2025 09:25
@dcherian dcherian requested a review from aulemahal August 18, 2025 16:51
@dcherian dcherian enabled auto-merge (squash) September 3, 2025 17:25
@dcherian dcherian merged commit f0b8083 into xarray-contrib:main Sep 3, 2025
13 checks passed
@larsbuntemeyer larsbuntemeyer deleted the cftime-bounds-fix branch September 4, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.10.7 breaks bounds_to_vertices for cftime objects
3 participants