Skip to content

Respect keep_attrs when using Dataset.set_index (#4955) #5972

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

Closed
wants to merge 1 commit into from

Conversation

gcaria
Copy link
Contributor

@gcaria gcaria commented Nov 10, 2021

The original issue was about DataArray.set_index, but since this simply calls Dataset.set_index, I have only added a test for the latter. DataArray.set_index probably deserves a test too, please let me know what you think.

@mathause
Copy link
Collaborator

#5957 seems to have snuck in - can you merge master?

@gcaria gcaria force-pushed the keep_attrs_set_index branch from 93b6b50 to c47cb38 Compare November 24, 2021 08:38
@mathause
Copy link
Collaborator

I just saw that the merge_indexes function gets removed in #5692. So I am not sure it makes sense to fix this here?

@benbovy FYI.

@benbovy
Copy link
Member

benbovy commented Nov 24, 2021

Thanks for pinging me @mathause. And sorry @gcaria I didn't see #4955 and this PR until now.

Indeed, there's no need for a fix considering that in #5692 variable attributes are now propagated properly through set_index and other operations on indexes (see 0086e32 and a891e22). Also, multi-index level coordinates become real coordinates so we don't need keep_attrs to handle the coordinate attributes, we can just propagate the attributes of each of the coordinates unambiguously.

@dcherian
Copy link
Contributor

Shall we merge only the test then?

@benbovy
Copy link
Member

benbovy commented Nov 24, 2021

I think I added tests for attributes in #5692, but I need to check.

@max-sixty
Copy link
Collaborator

Should we close?

@dcherian dcherian closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set_index does not respect keep_attrs
5 participants