Skip to content

Preserve all attrs with GroupBy by default. #7022

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

Merged
merged 2 commits into from
Sep 13, 2022

Conversation

dcherian
Copy link
Contributor

Closes #7012

When use_flox=False, we pass keep_attrs=None to Data*.reduce. This ends up delete Dataset and DataArray level attrs but preserves coordinate variable attrs. We now set the default to True always to match the default behaviour of apply_ufunc used by flox.xarray.xarray_reduce.

Closes pydata#7012

When `use_flox=False`, we pass `keep_attrs=None` to `Data*.reduce`.
This ends up delete Dataset and DataArray level attrs but preserves
coordinate variable attrs. We now set the default to True always to
match the default behaviour of `apply_ufunc` used by
`flox.xarray.xarray_reduce`.
Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -65,6 +65,9 @@ Bug fixes
By `András Gunyhó <https://github.com/mgunyho>`_.
- Avoid use of random numbers in `test_weighted.test_weighted_operations_nonequal_coords` (:issue:`6504`, :pull:`6961`).
By `Luke Conibear <https://github.com/lukeconibear>`_.
- Use ``keep_attrs=True`` in grouping and resampling operations by default (:issue:`7012`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think they were kept without flox - do you want to mention this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dataset level attrs were lost earlier:

attrs = self.attrs if keep_attrs else None

Now its always preserved whether you use flox or not. Should I clarify the second line here more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I clarify the second line here more.

Thanks for the clarification - fine to leave as is for me.

@@ -30,7 +30,7 @@ def dataset():
"foo": (("x", "y", "z"), np.random.randn(3, 4, 2)),
"baz": ("x", ["e", "f", "g"]),
},
{"x": ["a", "b", "c"], "y": [1, 2, 3, 4], "z": [1, 2]},
{"x": ("x", ["a", "b", "c"], {"name": "x"}), "y": [1, 2, 3, 4], "z": [1, 2]},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume there is a assert_identical somewhere below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes some existing tests failed when I added this.

@@ -65,6 +65,9 @@ Bug fixes
By `András Gunyhó <https://github.com/mgunyho>`_.
- Avoid use of random numbers in `test_weighted.test_weighted_operations_nonequal_coords` (:issue:`6504`, :pull:`6961`).
By `Luke Conibear <https://github.com/lukeconibear>`_.
- Use ``keep_attrs=True`` in grouping and resampling operations by default (:issue:`7012`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I clarify the second line here more.

Thanks for the clarification - fine to leave as is for me.

@dcherian dcherian enabled auto-merge (squash) September 13, 2022 15:08
@dcherian dcherian merged commit 63ba862 into pydata:main Sep 13, 2022
@dcherian dcherian deleted the groupby-attrs-True-always branch October 18, 2022 19:15
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.

Time-based resampling drops lat/lon coordinate metadata
2 participants