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.
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
combine_by_coordinates to handle unnamed data arrays. #4696
combine_by_coordinates to handle unnamed data arrays. #4696
Changes from all commits
b35de8e
f966e76
68b7b49
540961f
1c9b4c2
cb5ed5e
77020c0
6af896b
f06371a
7cdeabb
6190839
3055000
cbc002f
11a868b
89ac962
5f3afa5
feb90ce
db5b906
e884f52
0044bb9
44548ee
5fe8323
55f53b9
805145c
3eed47a
05faa88
6c75525
2c43030
f6fae25
81ec1ff
caaee74
637d4cc
b5940a1
d02da23
04cd5f8
e58a9e2
c0fc4f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is a nice way to refactor, that makes it easier to reason about the code.
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.
Can this Error ever be reached? Wouldn't the
stop this from being triggered?
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 intended this check to be part of the internal contract for
_combine_single_variable_hypercube
since it doesn't make sense to build an empty hypercube with this method. There would be no way to determine the variable involved, so a default sentinel name would have to be introduced.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.
can we also modify
combine_nested
so the two are consistent.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 wrote a test
test_combine_nested_unnamed_data_arrays
that passes a list of unnamedDataArray
s intocombine_nested
and it produces the expected output. Can you clarify what aboutcombine_nested
you want to be consistent?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 think I know what @dcherian meant - at first glance it looks like the
_combine_single_variable_hypercube
refactoring is a change that would also simplify the code incombine_nested
. But looking more closely I don't think it actually makes sense to do that, does it? It seems about as neat as it can be as is.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.
(It's considered bad practice to have mutable default arguments to functions in python.)
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 put this in because if someone calls this method with datasets as a named parameter the data_objects argument would be unspecified and their code would break with an unspecified argument error. This is part of the deprecation warning below.
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.
You make a good point, but that means the default argument should be
None
, not an empty list, asNone
is immutable.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.
Is renaming a breaking change?
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.
It technically is a breaking change - if someone was previously passing
combine_by_coords(datasets=...)
then this change will break their code. But renaming the argument does make sense with this PR. Not sure whether that means this justifies a deprecation cycle?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 think it probably does. But only a few extra lines and easy to copy from elsewhere.
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 have no idea what justifies a deprecation cycle in your project, or how one is performed. Can someone give me some guidance on this, seeing as this change will probably need one according to @max-sixty?
I also agree that renaming the argument makes sense here as data arrays and data sets are distinguished as two different things.
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.
No worries! (It's very briefly mentioned in our contributing guide, but maybe we should expand that...)
xarray has loads of regular users, and we don't want them to find that downloading a new version of xarray breaks their perfectly good code, even in some minor way. Therefore we normally hold their hand through any changes by warning them in the version before, or possibly by anticipating the way in which we might need to make sure that their old way of using the functions still works temporarily, to give them time to switch to our new way.
In this case the only people who could be affected are people who are currently passing
datasets
as a named argument tocombine_by_coords
, so I think we want to catch that specific possibility and tell them to change the named argument todata_objects
in future. So perhaps by adding the argument back in with a temporary check likeDoes that make sense?
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.
That makes sense. I'm somewhat concerned about users who ignore the warning (many software projects I've seen generate a lot of warnings), however I don't think there's much that can be done since the method signature will have to change anyway.
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.
Tiny clarification that this means datasets with other xarray.datarrays, not something about the numpy arrays inside the xarray.dataset objects.
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.
Good clarification.