Skip to content
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

Automatic naming causes trouble #2841

Closed
randolf-scholz opened this issue Mar 22, 2019 · 4 comments
Closed

Automatic naming causes trouble #2841

randolf-scholz opened this issue Mar 22, 2019 · 4 comments
Labels

Comments

@randolf-scholz
Copy link

Code Sample, a copy-pastable example if possible

A "Minimal, Complete and Verifiable Example" will make it much easier for maintainers to help you:
http://matthewrocklin.com/blog/work/2018/02/28/minimal-bug-reports

from xarray import Dataset, DataArray
ds = Dataset(coords={'foo':[1,2,3], 'bar':[1,2,3]})
da = DataArray([0], dims='test')
da.name = 'abc'
ds[(1,2)] = da
print(da.name)
print(ds[(1,2)].name)
print(type(ds[(1,2)].name))
print(ds[(1,2)])              # throws error

Problem description

The name gets overwritten with the tuple key, but is should stay intact?!?
Secondly the print statement throws an error because internally name_str = '%r ' % arr.name is called. But here arr.name is a tuple !!

Instead, it should be repr(arr.name) or F"{arr.name}"

Expected Output

The name should not change.

Output of xr.show_versions()

INSTALLED VERSIONS ------------------ commit: None python: 3.6.8 |Anaconda, Inc.| (default, Dec 30 2018, 01:22:34) [GCC 7.3.0] python-bits: 64 OS: Linux OS-release: 4.18.0-16-generic machine: x86_64 processor: x86_64 byteorder: little LC_ALL: None LANG: en_US.UTF-8 LOCALE: en_US.UTF-8 libhdf5: 1.10.4 libnetcdf: 4.6.1

xarray: 0.12.0
pandas: 0.24.2
numpy: 1.16.2
scipy: 1.2.1
netCDF4: 1.4.2
pydap: None
h5netcdf: None
h5py: 2.9.0
Nio: None
zarr: None
cftime: 1.0.3.4
nc_time_axis: None
PseudonetCDF: None
rasterio: None
cfgrib: None
iris: None
bottleneck: 1.2.1
dask: None
distributed: None
matplotlib: 3.0.3
cartopy: None
seaborn: None
setuptools: 40.8.0
pip: 19.0.3
conda: 4.6.8
pytest: None
IPython: 7.3.0
sphinx: 1.8.5

@shoyer shoyer added the bug label Mar 22, 2019
@shoyer
Copy link
Member

shoyer commented Mar 22, 2019

Thanks for the report.

Yes, this looks like a bug. A pull request to fix this would be very welcome.

@randolf-scholz
Copy link
Author

randolf-scholz commented Mar 25, 2019

@shoyer I had a look at xarray/core/formatting.py and it seems like there are a couple of places where keys get formatted this way. That is %-style formatting is used, ignoring the possibility that a dict key may be of type tuple. Personally I am really a big fan of f-strings, as they are faster and more readable than %-style formatting. They would in my opinion be one of the best ways fix this error and prevent it from happening again in the future.

In fact, the exact error that happened here is listed as one of the rationales for f-strings in the PEP: https://www.python.org/dev/peps/pep-0498/#rationale

Using them would of course mean dropping python 3.5 support.

@shoyer
Copy link
Member

shoyer commented Mar 25, 2019

I also love fstrings, but I don't think we're ready to drop Python 3.5 support yet. Let's try explicit calls to .format() for now.

dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.
dnowacki-usgs pushed a commit to dnowacki-usgs/xarray that referenced this issue Apr 18, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.

pep8

Remove positional indexes and fix !r formatting bug
max-sixty pushed a commit that referenced this issue Apr 19, 2019
Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.

pep8

Remove positional indexes and fix !r formatting bug
dcherian added a commit to yohai/xarray that referenced this issue Apr 19, 2019
* master: (29 commits)
  Handle the character array dim name  (pydata#2896)
  Partial fix for pydata#2841 to improve formatting. (pydata#2906)
  docs: Move quick overview one level up (pydata#2890)
  Manually specify chunks in open_zarr (pydata#2530)
  Minor improvement of docstring for Dataset (pydata#2904)
  Fix minor typos in docstrings (pydata#2903)
  Added docs example for `xarray.Dataset.get()` (pydata#2894)
  Bugfix for docs build instructions (pydata#2897)
  Return correct count for scalar datetime64 arrays (pydata#2892)
  Indexing with an empty array (pydata#2883)
  BUG: Fix pydata#2864 by adding the missing vrt parameters (pydata#2865)
  Reduce length of cftime resample tests (pydata#2879)
  WIP: type annotations (pydata#2877)
  decreased pytest verbosity (pydata#2881)
  Fix mypy typing error in cftime_offsets.py (pydata#2878)
  update links to https (pydata#2872)
  revert to 0.12.2 dev
  0.12.1 release
  Various fixes for explicit Dataset.indexes (pydata#2858)
  Fix minor typo in docstring (pydata#2860)
  ...
shoyer pushed a commit that referenced this issue May 16, 2019
* Partial fix for #2841 to improve formatting.

Updates formatting to use .format() instead of % operator. Changed all instances of % to .format() and added test for using tuple as key, which errored using % operator.

* Revert "Partial fix for #2841 to improve formatting."

This reverts commit f17f3ad.

* Implement load_dataset() and load_dataarray()

BUG: Fixes #2887 by adding @shoyer solution for load_dataset and load_dataarray, wrappers around open_dataset and open_dataarray which open, load, and close the file and return the Dataset/DataArray
TST: Add tests for sequentially opening and writing to files using new functions
DOC: Add to whats-new.rst. Also a tiny change to the open_dataset docstring

Update docstrings and check for cache in kwargs

Undeprecate load_dataset

Add to api.rst, fix whats-new.rst typo, raise error instead of warning
@keewis
Copy link
Collaborator

keewis commented Jan 25, 2021

the formatting of the tuple-typed key works now (#2906), so I'm closing this.

The name of da is still dropped, but that is intended behavior (it also happens with ds["def"] = da). For a discussion about what to do with tuple-typed keys see #4821.

@keewis keewis closed this as completed Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants