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 pickling issue #6249

Merged
merged 10 commits into from
Feb 10, 2022
Merged

Conversation

antarcticrainforest
Copy link
Contributor

@antarcticrainforest antarcticrainforest commented Feb 7, 2022

This PR tries to address some updates in the pandas library on how datetime objects are pickled. Since I was told that the datatype of the CFtimeIndex object will always be object I just instructed the __new__ method to receive any additional arguments but not to evaluate them. Which feels a little awkward but does the job. If there are better ways to deal with this please let me know. @spencerkclark @aidanheerdegen @mathause

@antarcticrainforest
Copy link
Contributor Author

Any idea why the pipeline is broken ?

@keewis
Copy link
Collaborator

keewis commented Feb 7, 2022

That's unrelated to this PR, you can safely ignore it.

scipy seems to have some issues with their setup (we're using their intersphinx registry, but the url for that returns a 404 error), so there's nothing we can do at the moment.

Edit: see #6250

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 @antarcticrainforest! I think this may be the best we can do in terms of a solution for now.

It's great that you added an integration test using distributed, since we've had issues like this crop up before. Maybe you could also add a pickling test within test_cftimeindex.py similar to @mathause's example, and a what's new entry as well?

@antarcticrainforest
Copy link
Contributor Author

Thanks @antarcticrainforest! I think this may be the best we can do in terms of a solution for now.

It's great that you added an integration test using distributed, since we've had issues like this crop up before. Maybe you could also add a pickling test within test_cftimeindex.py similar to @mathause's example, and a what's new entry as well?

Okidoke I've added a test for pickling cftimeindex objects and updated the whats-new document.

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! Looks good to me -- just a minor cleanup suggestion for the what's-new entry.

@spencerkclark spencerkclark added the plan to merge Final call for comments label Feb 10, 2022
@mathause mathause merged commit d994273 into pydata:main Feb 10, 2022
@mathause
Copy link
Collaborator

thanks @antarcticrainforest and welcome to xarray!

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.

open_mfdataset fails with cftime index when using parallel and dask delayed client
4 participants