-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Test suite: explicitly ignore irrelevant warnings #2162
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
Also includes a fix for Dataset.update()
xarray/core/merge.py
Outdated
for k, obj in other.items(): | ||
if isinstance(obj, DataArray): | ||
coord_names = [c for c in obj.coords | ||
if c not in obj.dims and c in dataset.coords] |
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.
E501 line too long (80 > 79 characters)
xarray/tests/test_dataset.py
Outdated
pandas_obj = a.to_pandas() | ||
ds_based_on_pandas = Dataset(pandas_obj) | ||
for dim in ds_based_on_pandas.data_vars: | ||
assert_array_equal(ds_based_on_pandas[dim], pandas_obj[dim]) |
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.
E501 line too long (80 > 79 characters)
@fujiisoup any thoughts? Do you agree that we should have different behavior for overriding |
I didn't notice that iterating over dataset gives a FutureWarning.
I actually do not yet understand well the difference between The docs says
but it does not. |
|
…rray into disable-serialization-warnings
OK, updated to only silence the warning in |
Do we update coordinates if other is Dataset? |
Yes, if other is a Dataset coordinates get overwritten. We probably already
had test coverage for this, but I also added some explicit tests.
…On Thu, May 24, 2018 at 9:21 PM Keisuke Fujii ***@***.***> wrote:
Do we update coordinates if other is Dataset?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2162 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1v2WCnW59lZwnW_nc2oQftwaZTgVks5t14bNgaJpZM4UFEGE>
.
|
I plan to merge this shortly (with a day or two) unless anyone else has comments |
if coord_names: | ||
other[k] = obj.drop(coord_names) | ||
if not isinstance(other, Dataset): | ||
other = OrderedDict(other) |
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 am slightly wondering this behavior may change after we finally support #884, i.e. we will not include coordinates in .key()
.
This line only applies to non-Dataset objects (see the isintance check on
the line above), so I think we are OK here.
…On Mon, May 28, 2018 at 9:05 PM Keisuke Fujii ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xarray/core/merge.py
<#2162 (comment)>:
> """
from .dataset import Dataset
from .dataarray import DataArray
- other = other.copy()
- for k, obj in other.items():
- if isinstance(obj, (Dataset, DataArray)):
- # drop duplicated coordinates
- coord_names = [c for c in obj.coords
- if c not in obj.dims and c in dataset.coords]
- if coord_names:
- other[k] = obj.drop(coord_names)
+ if not isinstance(other, Dataset):
+ other = OrderedDict(other)
I am slightly wondering this behavior may change after we finally support
#884 <#884>, i.e. we will not
include coordinates in .key().
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2162 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1gLkp5aD26C7wH18rk_zdfk9h5Kdks5t3Mj0gaJpZM4UFEGE>
.
|
I just missed this line. |
Includes silencing two warnings that show up in xarray's public API:
Dataset.update(Dataset)
Regression: Dataset.update(Dataset) #2161.equals()
with scalar timedelta64/datetime64 arrays containing NaT:Fixes #2161
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)