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

Add min_weight param to rolling_exp functions #8285

Merged
merged 9 commits into from
Oct 14, 2023

Conversation

max-sixty
Copy link
Collaborator

No description provided.

@Illviljan Illviljan added the run-benchmark Run the ASV benchmark workflow label Oct 8, 2023
Comment on lines +13 to +19
try:
import numbagg
from numbagg import move_exp_nanmean, move_exp_nansum

has_numbagg = numbagg.__version__
except ImportError:
has_numbagg = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a lazy import? Does our tests catch it?

Could you add numbagg here so we don't regress with regards to #6726:

https://github.com/pydata/xarray/blob/e8be4bbb961f58ba733852c998f2863f3ff644b1/xarray/tests/test_plugins.py#L216C9-L239

You can also use this for checking module availability:

def module_available(module: str) -> bool:
"""Checks whether a module is installed without importing it.
Use this for a lightweight check and lazy imports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a lazy import per se (it used to be, but I'm not sure that was necessarily deliberate vs. coincidental)

Using module_available doesn't seem to get the version?

What's the standard way of getting an optional module and checking the version in xarray? I'm happy to abide by the standard if there is one. The current approach does seem to work well in isolation...

Comment on lines 102 to 103
if has_numbagg is False or has_numbagg < "0.3.1":
raise ImportError("numbagg >= 0.3.1 is required for rolling_exp")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use

def mod_version(mod: ModType) -> Version:
"""Quick wrapper to get the version of the module."""
return _get_cached_duck_array_module(mod).version

To get the version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though this isn't a duck array module — numba / numbagg operates on numpy arrays. Would I still use this?

@max-sixty
Copy link
Collaborator Author

The tests fail because the build is cached on an old version of numbagg, we can try again when that's updated

@max-sixty
Copy link
Collaborator Author

Let me know any feedback re the version handling. Otherwise ready to go, will merge in the next day or so (and then happy to make any further changes from there if we don't catch something beforehand) — hope that's reasonable

@max-sixty max-sixty added the plan to merge Final call for comments label Oct 12, 2023
@max-sixty max-sixty merged commit dafd726 into pydata:main Oct 14, 2023
31 checks passed
@max-sixty max-sixty deleted the min-weight branch October 14, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments run-benchmark Run the ASV benchmark workflow topic-rolling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants