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

add a combine_attrs option to open_mfdataset #4971

Merged
merged 7 commits into from
Apr 3, 2021

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Feb 27, 2021

In order to fix the failing tests in #4902 we need to expose combine_attrs to be able to properly construct the expected result (to be passed through to the combine function).

This overlaps with the fallback of the attrs_file code, which I removed for now. Maybe combine_attrs="override" would be better?

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@mathause
Copy link
Collaborator

The motivation for attrs_file is given in #2382. Maybe require combine_attrs == "override" when attrs_file is not None?

The current behavior is combine_attrs="override", right? So when choosing something else this would have to be deprecated

@keewis
Copy link
Collaborator Author

keewis commented Feb 28, 2021

for the main object, yes, but for the variables of that object "drop" is currently always used. That means that if we try to sync both we introduce a breaking change.

We might eventually want to switch both to "drop_conflicts" so we would need to go through a deprecation cycle anyways.

@keewis
Copy link
Collaborator Author

keewis commented Mar 31, 2021

I decided to change the default from "drop" to "override", so the only thing that changes is that the attrs for variables will not be dropped but kept according to combine_attrs.

I guess that means that unless the CI fails this should be ready for review / merging?

@keewis
Copy link
Collaborator Author

keewis commented Mar 31, 2021

(the RTD build fails because docs.scipy.org is down)

@keewis
Copy link
Collaborator Author

keewis commented Apr 3, 2021

should we merge this? It is blocking #4902, which in turn is blocking #5041.

@keewis keewis merged commit 3cbd21a into pydata:master Apr 3, 2021
@keewis keewis deleted the open_mfdataset-combine_attrs branch April 3, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants