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

cftime: Fix resampling when time contains 0001-01-01 #9116

Merged
merged 9 commits into from
Aug 3, 2024

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Jun 13, 2024

cc @spencerkclark

@spencerkclark
Copy link
Member

Looks like CI confirms both of our experiences :).

I'm not sure why this would be platform-dependent...in the failing case I would be curious what ends up being returned from as_compatible_data in the traceback, and what step in as_compatible_data corrupts it. Unfortunately I don't think I have easy access to one of those platforms to do the debugging myself.

@dcherian
Copy link
Contributor Author

Thanks @spencerkclark this has been quite useful. I can dig in a bit more on my Mac.

@spencerkclark
Copy link
Member

Thanks @dcherian—I'll be curious what you find.

@dcherian dcherian changed the title Add test for #9108 cftime: Fix resampling when time contains 0001-01-01 Aug 1, 2024
@dcherian dcherian marked this pull request as ready for review August 1, 2024 13:00
@dcherian
Copy link
Contributor Author

dcherian commented Aug 1, 2024

Thanks @spencerkclark . Looks good to me!

doc/whats-new.rst Outdated Show resolved Hide resolved
@spencerkclark
Copy link
Member

Thanks @dcherian for taking a look and starting the what's new entry! I made a slight wording change, since I think the test_cftime_resample_gh_9108 case might work with the old code if the resampling frequency were "MS" instead of "ME"—maybe you can confirm on your machine.

It was good to be able to address this and remove some old cruft, e.g. the _days_in_month and _shift_month functions date all the way back to #2301, which was prior to the daysinmonth property being implemented on cftime objects (Unidata/cftime#138) and the addition of the has_year_zero option (Unidata/cftime#234).

I'll go ahead and merge at the end of the day tomorrow if there are no objections.

@spencerkclark spencerkclark merged commit 1ac19c4 into pydata:main Aug 3, 2024
28 checks passed
@spencerkclark
Copy link
Member

Thanks again for your help on this @dcherian

@dcherian dcherian deleted the fix-cftime-9108 branch November 17, 2024 16:15
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.

cftime resampling error
2 participants