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

time coding refactor #9906

Merged
merged 17 commits into from
Jan 2, 2025
Merged

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Dec 18, 2024

This introduces some changes in the encoding/decoding pipeline to prepare for other units beside ns. It also fixes some inconsistencies wrt the exact ns.resolution datetime64 bounds.

Finally it will reduce the "noise" over in #9618 quite substantial.

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@kmuehlbauer kmuehlbauer marked this pull request as ready for review December 18, 2024 13:47
doc/whats-new.rst Outdated Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this off @kmuehlbauer—these changes look good. Just a few very minor questions.

xarray/coding/times.py Outdated Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
@kmuehlbauer
Copy link
Contributor Author

@spencerkclark Thanks very much for the review. I've addressed according to your suggestions.

Copy link
Member

@spencerkclark spencerkclark 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 to me!

xarray/coding/times.py Outdated Show resolved Hide resolved
@kmuehlbauer
Copy link
Contributor Author

Thanks @spencerkclark for the review. I think, I've addressed the review items. Planning to merge this early next week.

@kmuehlbauer kmuehlbauer added the plan to merge Final call for comments label Jan 2, 2025
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

My brain hurts from trying to understand the differences between numpy and pandas times...

Thanks for the typing!

xarray/core/pdcompat.py Outdated Show resolved Hide resolved
xarray/core/pdcompat.py Outdated Show resolved Hide resolved
xarray/coding/times.py Outdated Show resolved Hide resolved
@kmuehlbauer
Copy link
Contributor Author

Thanks @headtr1ck for the review ❤️!

@dcherian dcherian merged commit 3fd79ac into pydata:main Jan 2, 2025
27 of 29 checks passed
@kmuehlbauer kmuehlbauer deleted the time-decoding-refactor branch January 2, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants