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

solve a bug when the units attribute is not a string #7085

Merged
merged 8 commits into from Sep 28, 2022

Conversation

ghislainp
Copy link
Contributor

@ghislainp ghislainp commented Sep 26, 2022

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

We faced a sort of bug with a colleague of mine. It seems to be legal to set a numeric value to the units attributes in an xarray or a netcdf file. xarray accepts to save such an array to netcdf: xr.DataArray([1, 2, 3], attrs={'units': 1}, name='x').to_csv('tmp.nc'). Reading this netcdf file with xarray.open_dataset raises an error.

It is unlikely to have a scalar for the units, but at least it happened to us (the value was NaN) and this raised an exception very difficult to understand.

This raises an exception because "since" in attrs["units"] was called twice in xarray codebase (in coding_times.py and in conventions.py) without checking for the type of the attribute.

This PR solves this improbable bug

@headtr1ck
Copy link
Collaborator

Looks good to me.
Feel free to add this bugfix to whats-new.

@headtr1ck headtr1ck added the plan to merge Final call for comments label Sep 26, 2022
@andersy005 andersy005 enabled auto-merge (squash) September 27, 2022 14:15

@requires_cftime
def test_scalar_unit() -> None:
# test that a scalar units (often NaN when using to_netcdf) does not raise an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Xarray actually write a nan for units? That seems problematic. Can you open a new issue witha reproducible example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened: #7093

@TomNicholas
Copy link
Contributor

Test failures seem unrelated (same failures with pyDAP occurred flakily in main) so I'm going to merge this shortly. Thanks @ghislainp !

@TomNicholas TomNicholas enabled auto-merge (squash) September 28, 2022 18:50
@TomNicholas TomNicholas merged commit 2c43663 into pydata:main Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants