Skip to content
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

Allow apply_ufunc to ignore missing core dims #8138

Merged
merged 19 commits into from
Sep 17, 2023

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Sep 1, 2023

This probably needs a review and another turn, maybe some more tests on multiple objects etc. There are a couple of questions inline.

@max-sixty
Copy link
Collaborator Author

Any thoughts on this, team?

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to think about how we handle multiple input_vars and what happens if multiple ds are passed (e.g apply_ufunc(func, ds1, ds2)). But that are probably rather edge-cases.

xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Show resolved Hide resolved
@max-sixty
Copy link
Collaborator Author

What's the best name for this arg?

  • missing_core_dim? Possibly too synonymous with other args which actually list dims, like exclude_dims?
  • on_missing_core_dim? Arguably more fluent with on_missing_core_dim="raise", but less standard...

@max-sixty max-sixty marked this pull request as ready for review September 12, 2023 01:03
@max-sixty
Copy link
Collaborator Author

OK, I'm reasonably happy with this now — it's ready to go, pending docs & changelog. Any thoughts prior?

@max-sixty
Copy link
Collaborator Author

We may have to think about how we handle multiple input_vars and what happens if multiple ds are passed (e.g apply_ufunc(func, ds1, ds2)). But that are probably rather edge-cases.

Sorry, just saw this. Yes, I actually had a TODO prior re adding tests for this, which I just added.

In the case of drop & raise, the order doesn't matter. In the case of copy, we copy from the first argument. Do you think that's reasonable?

xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/computation.py Outdated Show resolved Hide resolved
@max-sixty max-sixty merged commit 6b0d47c into pydata:main Sep 17, 2023
26 checks passed
@max-sixty max-sixty deleted the apply_func-missing branch September 17, 2023 08:20
max-sixty added a commit to max-sixty/xarray that referenced this pull request Sep 17, 2023
* Allow `apply_ufunc` to ignore missing core dims

* Display data returned in ufunc error message

This makes debugging much easier!

* Add tests for multiple var dataset

* Update xarray/core/computation.py

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>

* Update xarray/core/computation.py

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>

* more error message changes

---------

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apply_ufunc and Datasets with variables without the core dimension
3 participants