-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
xarray.core.variable.as_variable part of the public API #1422
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
Conversation
Many tests fail because they expect to have a |
Oh yes I haven't run the tests locally before submitting this, I didn't expected that. Mmm I don't see any current use of the |
Yes, we can safely remove the |
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 have a few minor suggestions, but in general I like this change!
xarray/core/variable.py
Outdated
---------- | ||
obj : object | ||
Object to convert into a Variable. | ||
If the object is already a Variable, return a shallow copy. |
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.
Can we keep these as bullet points for sphinx? I think that works for rendered docstrings if you're careful (e.g., see the docstring for xarray.Dataset.__init__
).
xarray/core/variable.py
Outdated
If all else fails, attempt to convert the object into a Variable by | ||
unpacking it into the arguments for creating a new Variable. | ||
name : str, optional | ||
Dimension name (only if data in `obj` is 1-dimensional). |
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 would say:
If provided:
obj
can be a 1D array, which is assumed to label coordinate values along a dimension of this given name.- Variables with name matching one of their dimensions are converted into
IndexVariable
objects.
doc/api.rst
Outdated
@@ -22,6 +22,7 @@ Top-level functions | |||
full_like | |||
zeros_like | |||
ones_like | |||
as_variable |
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 would move this to the "Advanced API" section at the bottom, next to Variable
and IndexVariable
.
xarray/core/variable.py
Outdated
- If all else fails, attempt to convert the object into a Variable by | ||
unpacking it into the arguments for creating a new Variable. | ||
name : str, optional | ||
If provided |
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.
add a final colon :
We have some tests for xarray/xarray/tests/test_variable.py Line 609 in d802484
The remaining test failures should be fixed by #1423 |
Also allow passing to `as_variable` objects that have a `data` attribute but no `values` attribute.
I've tried adding tests for lines in |
@benbovy thanks! |
xarray.core.variable.as_variable()
part of the public API? #1303git diff upstream/master | flake8 --diff
(if we ignore messages for .rst files and "imported but not used" messages forxarray.__init__.py
)whats-new.rst
for all changes andapi.rst
for new APIMake
xarray.core.variable.as_variable
part of the public API and accessible as a top-level function:xarray.as_variable
.I changed the docstrings to follow the numpydoc format more closely.
I also removed the
copy=False
keyword arguments as apparently it was unused.