Revert addition of fastpath causing incompatibilities with cftime
See discussion in #9138 Fixes #9138
This commit and pull request mostly serves as a staging group for a potential fix.
Test with:
pytest xarray/tests/test_cftimeindex.py::test_cftime_noleap_with_str
- [x] Closes #xxxx
- [x] Tests added
- [x] User visible changes (including notable bug fixes) are documented in
whats-new.rst - [x] New functions/methods are listed in
api.rst
The comment https://github.com/pydata/xarray/pull/9002/files#r1591214299 had the right intuition. The fastpath was the source of problems.
Empirically, i tested to see if it had any effects on the internal benchmarks that brought me to introducing it and it seems it does not (we avoid pandas quite heavily due to slowdowns it introduces, our data access is quite regular).
ps. i have visitors this week, so feel free to push to adjust anything you feel the need to change.
Not saying anything should change here, just giving the results of the benchmark
asv continuous main calendar_noleap --bench indexing
| Change | Before [42ed6d30] <main> | After [822c0b8e] <calendar_noleap> | Ratio | Benchmark (Parameter) |
|----------|----------------------------|--------------------------------------|---------|------------------------------------------------------------|
| + | 337±1μs | 618±1μs | 1.83 | indexing.AssignmentOptimized.time_assign_identical_indexes |
| + | 67.4±0.6μs | 77.7±0.5μs | 1.15 | indexing.HugeAxisSmallSliceIndexing.time_indexing |
| + | 139±0.7μs | 159±0.5μs | 1.14 | indexing.Assignment.time_assignment_basic('1slice') |
@pydata/xarray who is the best person to review this?
I believe the solution in https://github.com/pydata/xarray/issues/9138#issuecomment-2198145554 is a better path
Thanks for starting this @hmaarrfk—I went ahead and switched this to the approach suggested in https://github.com/pydata/xarray/issues/9138#issuecomment-2198145554 and moved the test to test_calendar_ops.py.
@aulemahal it would be great if you could take a look at this to ensure the test coverage is adequate. Thanks!
Thank you @spencerkclark glad this is moving forward!
Just reporting:
$ asv continuous main calendar_noleap --bench indexing
BENCHMARKS NOT SIGNIFICANTLY CHANGED.
@spencerkclark This change and the tests look good to me!
Thanks all!