issue_comments

@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.

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

I pushed a commit. Turns out reindex_variables also handles dict & scalar fill_value so it fixes both :)

@dcherian @keewis The resulting variables are now in order of appearance as suggested. This is backwards compatible and deterministic.

Needs decision: - convert fill_value to dict before calling align does not work - not sure how to use xarray.core.alignment.reindex_variables for reindexing

Thanks Deepak, I've already have some answers on your review. Will check the other things the next days.

Thanks Kai. I can take a look early next week. Sorry for the delay here, I just got back from a long vacation

Gentle ping @shoyer, as original issue author, and @dcherian for feedback. Any suggestions for improvement much appreciated. Thanks!

@Illviljan I'd prefer to handle benchmark in subsequent PR. Thanks for the links!

Thanks for bringing benchmarks back to life here @Illviljan. It's good to see that the changes from this PR do not affect the performance AFAICT.

It might make sense to add a benchmark for concatenation a large number of datasets with a large number of variables (with and without missing variables in some datasets) to check for performance of those special cases.

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!

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

@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.

@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!

@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.

@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.

Some typing suggestions.

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

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 :grinning:.

Hi, thanks for doing this @kmuehlbauer . FWIW I'm no longer seeing the issue I was previously seeing when I submitted https://github.com/pydata/xarray/pull/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.

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

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

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.

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?

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).