-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix: apply_ufunc with exclude_dims and vectorize #4130
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
Fix: apply_ufunc with exclude_dims and vectorize #4130
Conversation
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.
Thanks for working on this! What do you think of my suggestion of putting this in the to_gufunc_string
method rather than in the data model for _UFuncSignature
? That seems a little less invasive to me.
Sounds good. I can move |
Inplemented @shoyer's suggestion to add |
Unfortunately not. The build timed out after the error and the traceback is visible. My own setup doesn't time out on errors, so I asked support to help fix that. They removed stale memory constraints (which is why the traceback is visible), but somehow the build still times out on errors. In this case Edit: to re-trigger RTD, we have to close and reopen the PR; they don't allow rerunning a PR build |
Thanks keewis! I don't think I would have figured that out. I pushed the tags to my fork. |
But unfortunately this doesn't help :( If you have another idea I am all ears. But anyways it should not stop anyone from reviewing. |
on my own setup the build completes: https://readthedocs.org/projects/xarray-keewis/builds/11696307/ So this is definitely an issue with the setup, but I don't know how to fix that... |
@keewis In the xarray setup there is |
thanks, @kmuehlbauer, good spot. However, that seems to come from the PR build (see also https://readthedocs.org/projects/xray/builds/11695402/, which doesn't fail), so I guess there has to be something in the code that changes the version. |
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.
Very clean. Great work!
the merge seems to have fixed it. Not sure why it didn't work before. |
Thanks @keewis! |
…ub.com/mathause/xarray into fix/apply_ufunc_exclude_dims_vectorize
isort -rc . && black . && mypy . && flake8
whats-new.rst
for all changes andapi.rst
for new APIThis would be pretty useful when using
apply_ufunc
for e.g. statistical tests that requirevectorize=True
and can consume data of unequal length (i.e. usesexclude_dims
). This should also extend to dask arrays withdask="parallelized"
once #4060 is implemented.