-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Feature/support bn 1 1 #1278
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
Feature/support bn 1 1 #1278
Conversation
xarray/core/ops.py
Outdated
'move_max': 'max'} | ||
'move_max': 'max', 'move_var': 'var', | ||
'move_argmin': 'argmin', 'move_argmax': 'argmax', | ||
'move_rank': 'rank'} |
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.
I don't think we have a rank
method DataArray
/Dataset
, so we should probably remove this for now for consistency (though it would be nice to add it later!). Also I think it's untested.
xarray/core/ops.py
Outdated
setattr(cls, name, func) | ||
|
||
# bottleneck rolling methods | ||
if has_bottleneck: | ||
if LooseVersion(bn.__version__) < LooseVersion('1.0'): | ||
bn_version = LooseVersion(bn.__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.
It would be nice to drop old versions of bottleneck when feasible -- all this LooseVersion
stuff is really ugly and error prone (hard to test). But this is fine for now.
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.
Should we move the minimum bottleneck version to 1.1 and just get rid of all this then?
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.
Let's add a TODO to do that before releasing v0.10. But for now, we should keep it rather than updating the required version in a patch release (supposing we issue a v0.9 release).
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.
okay, done.
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.
Looks good to me -- feel free to merge at your pleasure!
* Added support for Dataset.rolling * Fixed an accidental bug in test_dataset.py * Dimension order is restored after Dataset.rolling. * Add some docstrings. Code clean up. * Make rolling_cls and groupby_cls private. * Removed another old-style call for the superclass method. * Fixed some breaks made by the previous merge. * Moved ImplementedArrayRolling into rolling * Made DatasetRolling utilize DataArrayRolling of each DataArrays. * Added 1 line to api.rst. * Updating test_dataset to catch #1278 * Test_dataset now tests bottleneck.move_var. * Raises an exception if no data_vars depend on the rolling-dims. * Fixed style issue and added docstring for inject_datasetrolling_methods * Recover unintended change. * An empty commit to trigger travicCI rebuild. * Recover test_assign_attrs which is deleted accidentaly. Some improvements based on shoyer's comments. * An empty commit to trigger Travis's rebuild
Bottleneck added support for rolling variance calculations (
bn.move_var
) as well as a few other functions in version 1.1. This PR conditionally uses those new functions, when they are available.git diff upstream/master | flake8 --diff
cc @shoyer and @d-chambers