-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
update minimum versions and associated code cleanup #2204
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.
Great, thanks @jhamman
doc/installing.rst
Outdated
@@ -41,7 +41,7 @@ For accelerating xarray | |||
For parallel computing | |||
~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|||
- `dask.array <http://dask.pydata.org>`__ (0.9.0 or later): required for | |||
- `dask.array <http://dask.pydata.org>`__ (0.XX or later): required for |
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.
was this intentional?
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 is the subject of #2203. I just wanted to make sure it was flagged for an update as part of this PR.
xarray/tests/__init__.py
Outdated
@@ -74,7 +74,7 @@ def _importorskip(modname, minversion=None): | |||
has_rasterio, requires_rasterio = _importorskip('rasterio') | |||
has_pathlib, requires_pathlib = _importorskip('pathlib') | |||
has_zarr, requires_zarr = _importorskip('zarr', minversion='2.2') | |||
has_np112, requires_np112 = _importorskip('numpy', minversion='1.12.0') | |||
has_np113, requires_np113 = _importorskip('numpy', minversion='1.12.0') |
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 this be minversion='1.13.0'
?
xarray/core/npcompat.py
Outdated
raise ValueError("axis=%i is invalid for the %i-dimensional " | ||
"input array" % (axis, m.ndim)) | ||
return m[tuple(indexer)] | ||
as_strided = np.lib.stride_tricks.as_strided |
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.
We could push this down into the caller, rather than maintain a layer here (but also fine as-is!)
ci/requirements-py27-min.yml
Outdated
- numpy==1.11 | ||
- pandas==0.18.0 | ||
- numpy==1.12 | ||
- pandas==0.20 |
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.
Anaconda doesn't have a pandas 0.19 build with numpy 1.12 so I've bumped another version to 0.20.
# Using `unittest.skipUnless` is a temporary workaround for pytest#568, | ||
# wherein class decorators stain inherited classes. | ||
# xref: xarray#1531, implemented in xarray #1557. | ||
func = unittest.skipUnless(has, reason='requires {}'.format(modname)) |
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.
This was fixed a few versions ago in pytest
@shoyer - when you get a chance, it may be worth looking over the failing travis build. I've pinned dask to 0.16 in the py35 build and am getting a handful of failures, particularly with dask distributed. |
I'm lost on how to finish this up. Even bumping dask to 0.16 and distributed to 1.20, I can't seem to get the python 3.5 tests to pass. Does anyone have thoughts on how best to proceed here? We can skip the failing tests but in my opinion, that defeats the purpose of setting a minimum version. |
Should we say that we require an even newer version of dask-distributed if you're using it, e.g., 1.21? If old versions of distributed don't work with xarray, one option would be to simply drop distributed from the "compatibility" build in Travis-CI. |
I managed to get the dask tests working here. This is ready to be merged. |
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.
LGTM :)
thanks @jhamman ! |
whats-new.rst
for all changes andapi.rst
for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)This updates the following minimum versions:
and drops our tests for python 3.4.