Fill missing data_vars during concat by reindexing

[kmuehlbauer]

• Closes Ignore missing variables when concatenating datasets? #508,
• Closes Add defaults during concat 508 #3545
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

This is another attempt to solve #508. Took inspiration from #3545 by @scottcha.

This follows along @dcherian's comment in the same above PR (#3545 (comment)).

Update:

After review the variable order is estimated by order of appearance in the list of datasets. That keeps full backwards compatibility and is deterministic. Thanks @dcherian and @keewis for the suggestions.

[kmuehlbauer]

There are no tests added currently. I'm just wondering, if that approach would work in general.

The current assumptions here:

• works only on data variables, not coordinates
• order is taken from first dataset, variables missing in first dataset are appended at the end
• missing variables are filled with fill_value by reindexing (thanks @dcherian for the inspiration).

[kmuehlbauer]

Thanks @Illviljan for activating the benchmark runs. Are those errors related to the changes? I'm not up to date with mypy, are these errors induced by changes here?

[Illviljan]

Benchmark is a numba issue, probably #7306.

Mypy is real, cannot getitem a object. Try out using isinstance instead of the try/except to narrow the typing.

[kmuehlbauer]

OK, green light's now also on mypy. Looks like the approach would work in general. Trying to add some tests now.

[kmuehlbauer]

This is ready for a first round of review. Thanks!

[scottcha]

Hi, thanks for doing this @kmuehlbauer . FWIW I'm no longer seeing the issue I was previously seeing when I submitted #3545 when I just run with released xarray v2022.12.0 (I haven't gone back further to see when the issue started going away so I'm not really sure if the old error has just been suppressed or if the single case I was seeing back then was resolved in a previous PR--or there is also a chance there is something which changed in my data over that long time period).

That being said I also applied this PR to my workflow and reran the concat code and it continues to pass correctly with this PR from what I've seen so far.

@kmuehlbauer did you see the tests I created here? https://github.com/pydata/xarray/blob/03f9b3b85aee039f47dd693322492ab09f57fb73/xarray/tests/test_concat.py
Not all of them got to a passing state but there were several cases I tried to document with the tests there.

[kmuehlbauer]

Thanks @scottcha for taking the time to testing things.

I'll have a look at your tests in more detail now. I've concentrated on my use case in the first place and hoped to get away with it 😀.

[Illviljan]

Some typing suggestions.

[kmuehlbauer]

Some typing suggestions.

Thanks @Illviljan, your suggestions and help is much appreciated.

[kmuehlbauer]

@scottcha I've found a glitch in the code due to your tests. Already pushed the changes here.

I'm going to cherry pick your tests here next.

[kmuehlbauer]

@scottcha I've tried to cherry-pick, but ended up copy/pasting and adding your authorship to the commit.

I think the final problem is the order in:

• test_concat_missing_multiple_consecutive_var
• test_multiple_datasets_with_multiple_missing_variables

These tests are flaky. Sometimes the order is correct and sometimes not. Can't immediately see the root cause here.

@Illviljan I'll try to add typing to these additional tests. Should be good for learning that.

[kmuehlbauer]

@Illviljan OK, I'm stuck now. I can't make anything out of the remaining mypy errors. Would be great if you could have another look here, thanks!

[kmuehlbauer]

@scottcha I think I've managed to get along with your tests. It looks like everything is running now.

One thing which is still unresolved:

• The order of data variables which are not available in the first dataset is not deterministic because of using set for gathering all variables. But maybe that can be neglected for now.

@Illviljan @dcherian This is ready for another round of review. Thanks for considering.

[kmuehlbauer]

I was hoping to gain something by merging the variable order code with _parse_datasets, to only have to traverse the datasets once.

The current behaviour, and the best I've come up so far in terms of performance:

1. count number of variables while iterating datasets (_parse_datasets)
2. check if first dataset contains all wanted variables
   2a. if that's the case, take the order from first dataset
3. check if the dataset with max count variables contains all wanted variables
   3a. if that's the case, take the order from that dataset
4. if not 2a or 3a, take order from first dataset and append missing variables to the end

[kmuehlbauer]

Finally, this is as far I could get with it. I'll leave it as is now. Looking forward for reviews and suggestions. Thanks @Illviljan for the great support!

[kmuehlbauer]

@dcherian rebased on latest main and fixed whats-new.rst. Should be good for another review.

[dcherian]

Thanks @kmuehlbauer and @scottcha . This is great to see finished! Very clean implementation in the end.

[kmuehlbauer]

@dcherian There slipped an old item from whats-new.rst back into. I've removed it. Should be OK now.

Great to see this functionality coming to next xarray version.