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

Complete deprecation of Dataset.dims returning dict #8980

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TomNicholas
Copy link
Member

@TomNicholas TomNicholas commented Apr 28, 2024

@TomNicholas
Copy link
Member Author

Interestingly changing the return type of .dims to a set breaks several tests, which seem to rely on dimension order. These tests should be changed, as they should never have relied on dimension order, and it might be easier to change them if we had #5733 implemented.

@dcherian
Copy link
Contributor

This feels a bit quick to me because this change had a pretty big blast radius. Shall we wait till September or so?

@TomNicholas
Copy link
Member Author

Happy to wait.

I just noticed that in the docstring for Dataset.transpose it says

the dataset dimensions themselves will remain in fixed (sorted) order.

Does that mean that returning a frozenset is not correct? And I should change it to return a sorted tuple instead??

@dcherian
Copy link
Contributor

dcherian commented May 1, 2024

Does that mean that returning a frozenset is not correct? And I should change it to return a sorted tuple instead??

I think we've discussed it before and decided that the unordered nature of set was a good hint to users to not depend on the ordering for anything.

@TomNicholas
Copy link
Member Author

Okay that's what I would want too.

But that does mean that Dataset.transpose will not change Dataset.dims... It can at most change the dims of the Variables wrapped by that Dataset.

@dcherian
Copy link
Contributor

dcherian commented May 1, 2024

But that does mean that Dataset.transpose will not change Dataset.dims... It can at most change the dims of the Variables wrapped by that Dataset.

This sounds fine to me. You can't rely on Dataset.dims for a dimensional ordering in the general case anyway.

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.

2 participants