-
-
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
Option to not auto-create index during expand_dims #8960
Option to not auto-create index during expand_dims #8960
Conversation
@dcherian I've addressed your comments, so would like to merge this. |
[also good opportunity to extend our stateful tests in a future PR if you want to dip your toes there ;) ] |
Co-authored-by: Deepak Cherian <[email protected]>
xarray/tests/test_dataset.py
Outdated
def test_expand_dims_create_index_data_variable(self, create_index_flag): | ||
# data variables should not gain an index ever | ||
ds = Dataset({"x": 0}) | ||
expanded = ds.expand_dims("x", create_index=create_index_flag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this raise a warning for create_index=True
since that option is effectively ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would that raise a warning? You think if a kwarg is irrelevant when passed it should raise a warning?
I also feel like that could create unnecessary warnings, especially if we made a global create_index
flag later like was discussed in #8872 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that if a user explicitly writes ds.expand_dims("x", create_index=True)
and we don't create an index, we should either warn or error.
In this case, it seems OK to warn and ask the user to call set_coords("x")
first. We want to encourage that behaviour anyway.
I have not thought about the global option yet, but usually we handle that by setting None
as the default value and then harmonizing with the global setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also OK punting that to a future PR. The current behaviour is pretty broken already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a UserWarning
in 7e8f895.
whats-new.rst
New functions/methods are listed inapi.rst
TODO:
DataArray.expand_dims