-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Dataset.dims should return a set, not a dict of sizes #8496
Comments
I agree — and given there is a clear alternative to (I thought there might be an issue for this but couldn't find one...) |
This is actually more than 5 minutes of effort: once I add the To remove these warnings I have to find every location where .dims was called but a dict was intended (but not places where .dims was called and only the keys used). I can't simply grep for that... |
Also if I warn on ds.dims, then even for future-proof uses of ds.dims (e.g
`for d in ds.dims`) the only way for the user to remove the warning is to
use `ds.sizes.keys()`, but that's not actually what we want them to do
eventually!
…On Thu, Nov 30, 2023, 5:47 PM Maximilian Roos ***@***.***> wrote:
I agree — and given there is a clear alternative to ds.dims, it would be
reasonable to put a FutureWarning on it and then change it in a few
versions.
(I thought there might be an issue for this but couldn't find one...)
—
Reply to this email directly, view it on GitHub
<#8496 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AISNPI5TSOV72JSNVHOFYQLYHEEJFAVCNFSM6AAAAABAB43R26VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZUGY4DAMZXGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Yeah that struck me too. It seems like a custom Mapping object would be the way to do. Raise warnings on |
Oh that's clever! I did not think of that. |
One insight I've learned form Stephan on this repo is to treat warnings in the test suite as a metric for the impact on users. This one seems pretty big! =) |
I hope that's a good thing 😅 @dcherian I think that |
Are we ready to complete this deprecation cycle? I ask because it would be nice to release |
What is your issue?
This is inconsistent:
Surely
ds.dims
should return something like aFrozenset({'x', 'y'})
? (because dimension order is meaningless when you have multiple arrays underneath - see #8498)The text was updated successfully, but these errors were encountered: