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

Taking a list of ds.dims does not raise warning #9799

Closed
hoxbro opened this issue Nov 19, 2024 · 6 comments
Closed

Taking a list of ds.dims does not raise warning #9799

hoxbro opened this issue Nov 19, 2024 · 6 comments

Comments

@hoxbro
Copy link
Contributor

hoxbro commented Nov 19, 2024

What is your issue?

Take the following example:

import xarray as xr
ds = xr.tutorial.open_dataset('air_temperature')

ds.dims["time"]  # Emits: The return type of `Dataset.dims` will be changed to return a set of ...
list(ds.dims)  # Does not emit a warning

I think __iter__ should also emit a warning, but I'm unsure if the decision not to do it is deliberate.

@hoxbro hoxbro added the needs triage Issue that has not been reviewed by xarray team member label Nov 19, 2024
@keewis
Copy link
Collaborator

keewis commented Nov 19, 2024

I think that's deliberate: we're trying to transition ds.dims from a dict to a set. The warning should thus only be emitted when trying to interact with ds.dims as a dict (such as when accessing the data through __getitem__), while __iter__ is also defined on a set and thus will continue to work after completing the deprecation cycle – although the question of reproducing the ordering remains for sets.

@hoxbro
Copy link
Contributor Author

hoxbro commented Nov 19, 2024

The keys in a dict are ordered, so if I do something like list(ds.dims)[0] or a variation of this, it will break (silently) when the switch happens.

But if this has already been discussed and a decision has been made, you can close the issue 👍

@keewis keewis removed the needs triage Issue that has not been reviewed by xarray team member label Nov 19, 2024
@shoyer
Copy link
Member

shoyer commented Nov 19, 2024

In principle, we could return an ordered set object from ds.dims in the future. We already use own wrapper objects for other dict-like properties in Xarray, mostly to add immutability.

@TomNicholas
Copy link
Member

The relevant PR was #8500. Which was almost a year ago, so maybe we should think about completing this deprecation cycle soon? I believe @keewis is correct, but agree with @shoyer that we might actually prefer to return an OrderedSet.

@keewis
Copy link
Collaborator

keewis commented Nov 19, 2024

I also think we should use a OrderedSet, as otherwise maintaining doctests for Dataset will become very tricky (plus I recently learned that we use ds.dims as the default dimension order in Dataset.to_dataframe, so this definitely shouldn't be effectively random)

@TomNicholas
Copy link
Member

If we're in agreement then I think the original complaint won't apply and we can close this?

@keewis keewis closed this as completed Nov 19, 2024
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

No branches or pull requests

4 participants