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 for date offset strings with resample loffset #8422

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

kafitzgerald
Copy link
Contributor

Closes #8399.

Reverting this minor change from #7206 allows for offset aliases to again be used with the loffset argument to resample.

Hopefully this doesn't break anything from that older PR (all tests pass). It wasn't entirely clear to me why the change was made initially. Perhaps just to simplify and this was an oversight?

Copy link

welcome bot commented Nov 7, 2023

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@dcherian
Copy link
Contributor

dcherian commented Nov 7, 2023

Thanks @kafitzgerald It's great to see you here!

It wasn't entirely clear to me why the change was made initially. Perhaps just to simplify and this was an oversight?

Yes probably.

Can you add a test please?

@kafitzgerald
Copy link
Contributor Author

Sorry about that.

And it turns out covering this particular case was fairly easy as well - just an additional parameter for the test_resample_loffset_cftimeindex test to cover date offset strings. Let me know if you had something else in mind.

I did confirm it fails prior to the change and passes after.

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 @kafitzgerald — this looks perfect to me! I forgot this was the way things had been implemented before (it points to the importance of this test). Could you also add a bug fix what's new entry to the documentation?

@kafitzgerald
Copy link
Contributor Author

Will do!

@dcherian
Copy link
Contributor

dcherian commented Nov 7, 2023

Great thanks @kafitzgerald . Welcome to Xarray! It's great!

@dcherian dcherian enabled auto-merge (squash) November 7, 2023 23:49
@max-sixty
Copy link
Collaborator

Thanks @kafitzgerald ! Welcome to Xarray!

@dcherian dcherian disabled auto-merge November 8, 2023 00:52
@dcherian dcherian merged commit feba698 into pydata:main Nov 8, 2023
26 of 28 checks passed
Copy link

welcome bot commented Nov 8, 2023

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

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.

Dataset.resample fails with certain time offset strings provided to the loffset parameter
4 participants