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

open_mfdataset: Raise if combine='by_coords' and concat_dim=None #5231

Merged

Conversation

TomNicholas
Copy link
Member

@TomNicholas TomNicholas commented Apr 28, 2021

Fixes bug which allowed incorrect arguments to be passed to open_mfdataset without complaint.

The combination open_mfdataset(files, combine='by_coords', concat_dim='t') should never have been permitted, and in fact it wasn't permitted until the last part of the deprecation process from the old auto_combine. It makes no sense to pass this combination because the combine_by_coords function does not have a concat_dim argument at all!

The effect was pretty benign - the concat_dim arg wasn't really used for anything in that case, and the result of passing dodgy datasets would just be a less informative error. However there were multiple tests which assumed this behaviour was okay - I had to remove that particular parametrization for a bunch of your join tests @dcherian because they now fail with a different (clearer) error.

I also noticed a related issue which I fixed - internally open_mfdataset was performing a rearrangement of the input datasets that it needs for the case combine='nested', even in the case combine='by_coords'. I hadn't previously realised that we can just skip this rearrangement without issue, so open_mfdataset(combine='by_coords') should be a little bit faster now.

@TomNicholas TomNicholas requested a review from dcherian April 28, 2021 19:16
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 just one minor comment.

xarray/tests/test_backends.py Outdated Show resolved Hide resolved
xarray/tests/test_backends.py Outdated Show resolved Hide resolved
@TomNicholas TomNicholas mentioned this pull request Apr 29, 2021
13 tasks
@dcherian
Copy link
Contributor

flaky pydap tests being flakey...

Thanks @TomNicholas

@dcherian dcherian changed the title Prevent passing combine='by_coords' with concat_dim not none to open_mfdataset open_mfdataset: Raise if combine='by_coords' and concat_dim=None Apr 30, 2021
@dcherian dcherian merged commit 01b6cc7 into pydata:master Apr 30, 2021
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.

Same files in open_mfdataset() unclear error message
2 participants