-
-
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
Merged
TomNicholas
merged 18 commits into
pydata:main
from
TomNicholas:expand_dims_create_1d_index
Apr 27, 2024
Merged
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
3719ba7
test expand_dims doesn't create Index
TomNicholas 018e74b
add option to not create 1D index in expand_dims
TomNicholas f680505
refactor tests to consider data variables and coordinate variables se…
TomNicholas f10509a
fix bug causing new test to fail
TomNicholas 8152c0a
test index auto-creation when iterable passed as new coordinate values
TomNicholas aa813cf
make test for iterable pass
TomNicholas e78de7d
added kwarg to dataarray
TomNicholas b1329cc
whatsnew
TomNicholas a9f7e0c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 62e750f
update tests to use private versions of assertions
TomNicholas f86c82f
create_1d_index->create_index
TomNicholas 4dd8d3c
Merge branch 'main' into expand_dims_create_1d_index
TomNicholas d5d90fd
Update doc/whats-new.rst
TomNicholas 7e8f895
warn if create_index=True but no index created because dimension vari…
TomNicholas ed85446
add string marks in warning message
TomNicholas 39571ba
Merge branch 'main' into expand_dims_create_1d_index
TomNicholas 928dbe9
Merge branch 'main' into expand_dims_create_1d_index
TomNicholas 6bde158
fix bad merge
TomNicholas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.