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.

Sign up for GitHub

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

Jump to bottom

keep_attrs for pad #7267

Merged
merged 15 commits into from Dec 12, 2022
Merged

keep_attrs for pad #7267

merged 15 commits into from Dec 12, 2022

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Nov 8, 2022 •
edited

I ran into this while trying DataTree.pad, which silently dropped the attrs, even with keep_attrs=True.

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@keewis
Copy link
Collaborator Author

keewis commented Nov 8, 2022 •
edited

wow, it seems now the mypy CI fails with a segfault. @headtr1ck, @Illviljan, @max-sixty: should we pin mypy to mypy<0.990 until we (or rather: any of you?) can figure out what is going wrong with that?

dcherian
dcherian approved these changes Nov 8, 2022
View reviewed changes
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.

LGTM. Could consider having it be True by default since most attrs will still make sense.

@max-sixty
Copy link
Collaborator

LGTM. Could consider having it be True by default since most attrs will still make sense.

+1 to both the specific point and generally moving in this direction...

dcherian
dcherian reviewed Nov 8, 2022
View reviewed changes
xarray/core/variable.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Nov 8, 2022
View reviewed changes
xarray/core/dataset.py Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator Author

keewis commented Nov 8, 2022

I'm always setting xr.set_options(keep_attrs=True) after importing xarray, anyways, so 👍 from me.

However, this would be a change in policy (#3891) so I wonder if we should do this in one (slightly big) PR? But then we might need a deprecation cycle?

@dcherian
Copy link
Contributor

so I wonder if we should do this in one (slightly big) PR? But then we might need a deprecation cycle?

I think we decided to avoid the deprecation cycle for keep_attrs changes.

@keewis keewis mentioned this pull request Nov 29, 2022
Closed
4 tasks
@dcherian
Copy link
Contributor

dcherian commented Dec 7, 2022

@keewis shall we merge?

@headtr1ck headtr1ck added plan to merge Final call for comments topic-metadata Relating to the handling of metadata (i.e. attrs and encoding) labels Dec 10, 2022
@keewis
Copy link
Collaborator Author

keewis commented Dec 12, 2022

the mypy failures look unrelated, so let's merge

@keewis keewis merged commit db68db6 into pydata:main Dec 12, 2022
@keewis keewis deleted the pad-keep_attrs branch December 12, 2022 15:59
@keewis keewis linked an issue Dec 12, 2022 that may be closed by this pull request
Dataset.pad() does not respect individual data array attributes #7332
Closed
4 tasks
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 18, 2023
@dcherian
Merge branch 'main' into kvikio
426519f 
* main: (41 commits)
  v2023.01.0 whats-new (pydata#7440)
  explain keep_attrs in docstring of apply_ufunc (pydata#7445)
  Add sentence to open_dataset docstring (pydata#7438)
  pin scipy version in doc environment (pydata#7436)
  Improve performance for backend datetime handling (pydata#7374)
  fix typo (pydata#7433)
  Add lazy backend ASV test (pydata#7426)
  Pull Request Labeler - Workaround sync-labels bug (pydata#7431)
  see also : groupby in resample doc and vice-versa (pydata#7425)
  Some alignment optimizations (pydata#7382)
  Make `broadcast` and `concat` work with the Array API (pydata#7387)
  remove `numbagg` and `numba` from the upstream-dev CI (pydata#7416)
  [pre-commit.ci] pre-commit autoupdate (pydata#7402)
  Preserve original dtype when accessing MultiIndex levels (pydata#7393)
  [pre-commit.ci] pre-commit autoupdate (pydata#7389)
  [pre-commit.ci] pre-commit autoupdate (pydata#7360)
  COMPAT: Adjust CFTimeIndex.get_loc for pandas 2.0 deprecation enforcement (pydata#7361)
  Avoid loading entire dataset by getting the nbytes in an array (pydata#7356)
  `keep_attrs` for pad (pydata#7267)
  Bump pypa/gh-action-pypi-publish from 1.5.1 to 1.6.4 (pydata#7375)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

Assignees
No one assigned
Labels
plan to merge Final call for comments topic-metadata Relating to the handling of metadata (i.e. attrs and encoding)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dataset.pad() does not respect individual data array attributes
5 participants
@keewis @max-sixty @dcherian @headtr1ck @Illviljan