conditionally disable bottleneck

[keewis]

As this came up in #5424 (and because I can't seem to reliably reproduce expected values in #4972 if it is enabled) this adds a option to disable bottleneck, even if it is installed.

In #5424 it was suggested to also allow replacing bottleneck with numbagg. If that's something we want (and numbagg supports the operations we need) I can try looking into renaming the new option to something like xr.set_options(accelerate_with="bottleneck") (or something else, if someone has a great idea).

Tests are missing because I have no idea how to check that this works (except by mocking bottleneck).

• Closes Bottleneck bug with unusual strides - causes segfault or wrong number #5424
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst

[github-actions]

Unit Test Results

         6 files  ±0           6 suites  ±0   57m 2s ⏱️ ±0s
16 217 tests ±0  14 483 ✔️ ±0  1 734 💤 ±0  0 ❌ ±0 
90 498 runs  ±0  82 324 ✔️ ±0  8 174 💤 ±0  0 ❌ ±0 

Results for commit 3956b73. ± Comparison against base commit 3956b73.

♻️ This comment has been updated with latest results.

[dcherian]

We use it in other places too (for e.g.):

xarray/xarray/core/nputils.py

Lines 139 to 147 in c472f8a

	if (
	_USE_BOTTLENECK
	and isinstance(values, np.ndarray)
	and bn_func is not None
	and not isinstance(axis, tuple)
	and values.dtype.kind in "uifc"
	and values.dtype.isnative
	and (dtype is None or np.dtype(dtype) == values.dtype)
	):

xr.set_options(accelerate_with="bottleneck")

I like this idea but we should wait for more input.

[max-sixty]

Nice!

pandas uses use_bottleneck — and same for numba / numexpr.

To what extent would bottleneck & numba be mutually exclusive? Until everything is implemented in numbagg, I guess they won't be, and we might want separate options. Having a single option would be nicer otherwise.

[keewis]

as discussed in the meeting, we will keep use_bottleneck for now. I still don't know how to test this, but otherwise this should be ready for reviews and merging.

Edit: we also don't have alternatives for functions like duck_array_ops.push (used in ffill etc.) so disabling bottleneck would fail the function. Should we just silently use bottleneck, or raise an error?

[dcherian]

LGTM. I guess you would have to mock bottleneck to properly test this.

[dcherian]

so disabling bottleneck would fail the function. Should we just silently use bottleneck, or raise an error?

Ah this is a good test!

with xr.set_options(use_bottleneck=False):
	with pytest.raises(...):
		dataarray.ffill()

IMO it should raise an error so that use_bottleneck is a "global" control on whether xarray uses bottleneck or not. The context manager gives the user some flexibility to opt-in to using bottleneck where they want to.

[keewis]

I'm not sure how to test rolling: monkeypatching bottleneck.move_sum does not work because Rolling only accesses that on import, i.e. before the test is executed.

Everything else is should be done, though.

[dcherian]

just a suggestion on the error message.

Can you open a new issue about testing for rolling? I think it is OK to merge without that.

[dcherian]

Thanks @keewis. We can extend the tests for rolling later.