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

Global option to always keep/discard attrs on operations #2482

Merged
merged 15 commits into from
Oct 30, 2018
Merged

Global option to always keep/discard attrs on operations #2482

merged 15 commits into from
Oct 30, 2018

Conversation

TomNicholas
Copy link
Member

@TomNicholas TomNicholas commented Oct 12, 2018

Adds a global option to either always keep or always discard attrs in method and function calls.

The behaviour is backwards-compatible, as the logic is:

  • if keep_attrs supplied as keyword argument then use that
  • else if global option (xarray.set_options(keep_attrs=True)) is set then use that
  • else use default value of keep_attrs argument for that particular function/method (kept the same as they were for backwards-compatibility).

Main use cases include users who want to store the units of their data in the attrs, users who want to always keep information about the source or history of their data, and users who want to store objects in their attributes which are needed to supplement the xarray objects (e.g. an xgcm.grid). It should eventually be superceded by hooks for custom attribute handling (#988), but will be useful until then.

I have left the top-level functions like concat and merge alone. Currently concat keeps the attributes of the first object passed to it, and merge returns a dataset with no attributes. It's not clear how this should be treated though, so I left it to users to extend those functions if they need to.

@pep8speaks
Copy link

pep8speaks commented Oct 12, 2018

Hello @TomNicholas! Thanks for updating the PR.

Line 56:80: E501 line too long (97 > 79 characters)

Comment last updated on October 29, 2018 at 15:19 Hours UTC

xarray/core/common.py Outdated Show resolved Hide resolved
Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

I still need to look more carefully at the tests here....

xarray/core/common.py Outdated Show resolved Hide resolved
xarray/tests/test_options.py Outdated Show resolved Hide resolved
@shoyer
Copy link
Member

shoyer commented Oct 18, 2018

Unit tests and docs look good.

@TomNicholas
Copy link
Member Author

I wanted to clean up my commits using git rebase, but when I tried git rebase -i I only had the choice to alter my last 5 commits to this branch, not all 8. I'm not sure if you know what I'm doing wrong there?

@shoyer
Copy link
Member

shoyer commented Oct 18, 2018

@TomNicholas I'm not sure, but don't worry about cleaning up git history with rebase. We squash all commits before merging anyways, and GitHub's review tools work a little better when you don't rebase.

@TomNicholas TomNicholas reopened this Oct 27, 2018
@TomNicholas
Copy link
Member Author

No idea why the CI test fails now - I literally changed one line, from class TestAttrRetention: to
class TestAttrRetention(object):, and all tests pass on my local machine.

@spencerkclark
Copy link
Member

@TomNicholas it looks like the merge conflict in whats-new.rst is preventing the CI from running. I think if you sync with the latest master branch and resolve the conflict things should work properly.

@spencerkclark
Copy link
Member

@TomNicholas thanks, things look better now. Don't worry about the Appveyor failures; a new version of cftime (installed via pip on the Python 2 builds there) was just released and there is a bug they are still ironing out on a few platforms (including Windows): conda-forge/cftime-feedstock#10, Unidata/cftime#76.

doc/whats-new.rst Outdated Show resolved Hide resolved
xarray/core/options.py Outdated Show resolved Hide resolved
xarray/core/options.py Outdated Show resolved Hide resolved
xarray/core/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @TomNicholas. There still seems to be a few redundant instances of the if block.

xarray/core/dataarray.py Outdated Show resolved Hide resolved
xarray/core/dataarray.py Outdated Show resolved Hide resolved
xarray/core/dataset.py Outdated Show resolved Hide resolved
xarray/core/groupby.py Outdated Show resolved Hide resolved
@shoyer shoyer merged commit 6d55f99 into pydata:master Oct 30, 2018
@shoyer
Copy link
Member

shoyer commented Oct 30, 2018

Thanks @TomNicholas !

dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 30, 2018
* master:
  Global option to always keep/discard attrs on operations (pydata#2482)
  Remove tests where answers change in cftime 1.0.2.1 (pydata#2522)
  Finish deprecation cycle for DataArray.__contains__ checking array values (pydata#2520)
  Fix bug where OverflowError is not being raised (pydata#2519)
dcherian pushed a commit to yohai/xarray that referenced this pull request Dec 16, 2018
* upstream/master: (122 commits)
  add missing , and article in error message (pydata#2557)
  Add libnetcdf, libhdf5, pydap and cfgrib to xarray.show_versions() (pydata#2555)
  revert to dev version for 0.11.1
  Release xarray v0.11
  DOC: update whatsnew for xarray 0.11 release (pydata#2548)
  Drop the hack needed to use CachingFileManager as we don't use it anymore. (pydata#2544)
  add full test env for py37 ci env (pydata#2545)
  Remove old-style resample example in documentation (pydata#2543)
  Stop loading tutorial data by default (pydata#2538)
  Remove the old syntax for resample. (pydata#2541)
  Remove use of deprecated, unused keyword. (pydata#2540)
  Deprecate inplace (pydata#2524)
  Zarr chunking (GH2300) (pydata#2487)
  Include multidimensional stacking groupby in docs (pydata#2493) (pydata#2536)
  Switch enable_cftimeindex to True by default (pydata#2516)
  Raise more informative error when converting tuples to Variable. (pydata#2523)
  Global option to always keep/discard attrs on operations (pydata#2482)
  Remove tests where answers change in cftime 1.0.2.1 (pydata#2522)
  Finish deprecation cycle for DataArray.__contains__ checking array values (pydata#2520)
  Fix bug where OverflowError is not being raised (pydata#2519)
  ...
@TomNicholas TomNicholas deleted the keep_attrs_global branch February 2, 2019 23:50
@amcnicho amcnicho mentioned this pull request Feb 27, 2020
4 tasks
@TomNicholas TomNicholas added the topic-metadata Relating to the handling of metadata (i.e. attrs and encoding) label Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-metadata Relating to the handling of metadata (i.e. attrs and encoding)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants