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.

Sign up for GitHub

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

Jump to bottom

Refactor parse_cf to not modify coordinates inplace #1285

Merged
merged 2 commits into from
Jan 12, 2020
Merged

Refactor parse_cf to not modify coordinates inplace #1285

merged 2 commits into from
Jan 12, 2020

Conversation

jthielen
Copy link
Collaborator

Description Of Changes

Refactors parse_cf to not modify coordinates (which may be tied to immutable indexes) inplace, and instead reassign the coordinates in the recommended way. If this goes as my tests locally lead me to believe, this should fix the issues created with xarray in pydata/xarray#3519.

Checklist

@dopplershift dopplershift added Area: Xarray Pertains to xarray integration Type: Maintenance Updates and clean ups (but not wrong) labels Jan 10, 2020
@dopplershift dopplershift added this to the 1.0 milestone Jan 10, 2020
dopplershift
dopplershift previously approved these changes Jan 12, 2020
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

I like the implementation approach taken here. Very clean to assemble the new coords and assign once.

@dopplershift
Copy link
Member

I'll rebase and merge.

@dopplershift
Copy link
Member

Oh, no this was already up to date. The test failure was real, failing with our oldest supported xarray. It looks like assign_coords() only recent accepted passing a dict-like, and before only accepted **kwargs. Looks like an easy tweak that should work across versions. I've pushed a commit to fix. Hopefully this passes.

@dopplershift
Copy link
Member

Ignoring Codacy and merging.

@dopplershift dopplershift merged commit ce72871 into Unidata:master Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift dopplershift left review comments

Assignees
No one assigned
Labels
Area: Xarray Pertains to xarray integration Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

Issues with XArray master
2 participants
@jthielen @dopplershift