Don't drop unreduced variables #5393
Conversation
|
Great! Thanks @malmans2 ! |
|
I'll leave this open for a day or so, as this is the sort of area where I'm going to miss some intricacy that someone else is familiar with. |
Co-authored-by: keewis <[email protected]>
| else: | ||
| if ( | ||
| not numeric_only | ||
| not reduce_dims |
There was a problem hiding this comment.
actually, wouldn't it make more sense to add unreduced variables without calling reduce? Something like
if not reduce_dims:
variables[name] = var
elif not numeric_only or np.issubdtype(var.dtype, nb.number) or var.dtype == np.bool_:
# ...
variables[name] = var.reduce(...)There was a problem hiding this comment.
That was my first idea, but a few test failed, so it would be a breaking change. I think it mainly has to do with how attributes are handled by reduce.
There was a problem hiding this comment.
well, attrs seems to be one of the issues (but one we could fix!). However, some reduction functions (e.g. std, var) need to run on variables that don't have the reduce dims, so my suggestion is actually wrong. See also the concept of invariant_0d introduced in #5207.
As I'm sure I will have forgotten about this in a few months: could you add a comment explaining this?
There was a problem hiding this comment.
last question: if I'm understanding this correctly, not numeric_only and all following lines will be overridden by not reduce_dims. Should we move that three lines down, below the dtype checks?
|
@malmans2 would you like to add a whatsnew? |
|
@malmans2 thank you and please feel free to add a whatsnew in another PR! |
3 participants
pre-commit run --all-fileswhats-new.rstReduce methods such as
meandrop non-numeric variables. However, there's no need to drop variables that are not actually reduced (i.e., when"reduce_dim" not in da.dims).cc: @rcaneill