Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix time indexing regression in convert_calendar #9192

Merged
merged 8 commits into from
Jul 11, 2024

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Jun 29, 2024

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
  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@hmaarrfk hmaarrfk marked this pull request as draft June 29, 2024 16:02
See discussion in pydata#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
```
@hmaarrfk hmaarrfk changed the title MRC -- Selecting with string for cftime Revert addition of fastpath causing incompatibilities with cftime Jun 29, 2024
@hmaarrfk
Copy link
Contributor Author

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 hmaarrfk marked this pull request as ready for review June 29, 2024 18:36
@hmaarrfk
Copy link
Contributor Author

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

@dcherian dcherian marked this pull request as draft June 29, 2024 20:08
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jul 2, 2024

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

@max-sixty
Copy link
Collaborator

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

@dcherian
Copy link
Contributor

dcherian commented Jul 2, 2024

I believe the solution in #9138 (comment) is a better path

@spencerkclark
Copy link
Member

Thanks for starting this @hmaarrfk—I went ahead and switched this to the approach suggested in #9138 (comment) 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!

@hmaarrfk hmaarrfk marked this pull request as ready for review July 7, 2024 18:28
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jul 7, 2024

Thank you @spencerkclark glad this is moving forward!

@spencerkclark spencerkclark changed the title Revert addition of fastpath causing incompatibilities with cftime Fix time indexing regression in convert_calendar Jul 7, 2024
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jul 8, 2024

Just reporting:

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

@aulemahal
Copy link
Contributor

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

@dcherian dcherian merged commit ff15a08 into pydata:main Jul 11, 2024
28 checks passed
@dcherian
Copy link
Contributor

Thanks all!

dcherian added a commit to dcherian/xarray that referenced this pull request Jul 11, 2024
* main:
  exclude the bots from the release notes (pydata#9235)
  switch the documentation to run with `numpy>=2` (pydata#9177)
  `numpy` 2 compatibility in the iris code paths (pydata#9156)
  `numpy` 2 compatibility in the `netcdf4` and `h5netcdf` backends (pydata#9136)
  Fix time indexing regression in `convert_calendar` (pydata#9192)
  Use duckarray assertions in test_coding_times (pydata#9226)
  Use reshape and ravel from duck_array_ops in coding/times.py (pydata#9225)
  Cleanup test_coding_times.py (pydata#9223)
  Only use necessary dims when creating temporary dataarray (pydata#9206)
  Fix two bugs in DataTree.update() (pydata#9214)
  Use numpy 2.0-compat `np.complex64` dtype in test (pydata#9217)
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.

Can't select time with str after converting the calendar to noleap
5 participants