xarray icon indicating copy to clipboard operation
xarray copied to clipboard

Revert addition of fastpath causing incompatibilities with cftime

Open hmaarrfk opened this issue 1 year ago • 5 comments

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

hmaarrfk avatar Jun 29 '24 16:06 hmaarrfk

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).

hmaarrfk avatar Jun 29 '24 18:06 hmaarrfk

ps. i have visitors this week, so feel free to push to adjust anything you feel the need to change.

hmaarrfk avatar Jun 29 '24 19:06 hmaarrfk

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')        |

hmaarrfk avatar Jul 02 '24 01:07 hmaarrfk

@pydata/xarray who is the best person to review this?

max-sixty avatar Jul 02 '24 02:07 max-sixty

I believe the solution in https://github.com/pydata/xarray/issues/9138#issuecomment-2198145554 is a better path

dcherian avatar Jul 02 '24 03:07 dcherian

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!

spencerkclark avatar Jul 07 '24 18:07 spencerkclark

Thank you @spencerkclark glad this is moving forward!

hmaarrfk avatar Jul 07 '24 18:07 hmaarrfk

Just reporting:

$ asv continuous main calendar_noleap --bench indexing
BENCHMARKS NOT SIGNIFICANTLY CHANGED.

hmaarrfk avatar Jul 08 '24 03:07 hmaarrfk

@spencerkclark This change and the tests look good to me!

aulemahal avatar Jul 08 '24 14:07 aulemahal

Thanks all!

dcherian avatar Jul 11 '24 02:07 dcherian