issue_comments

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.

I'm still along and yes I do still need this functionality (I still sync back to this PR when I have data missing vars). The issue was that the technical requirements got beyond what I was able to account for with the time I had available. If you or someone else was interested in picking it up I'd be happy to evaluate against my use cases.

I just pushed an incomplete set of changes as @kmuehlbauer tests have demonstrated there was some incomplete cases the PR still isn't handling.
Here is a summary: 1. I've simplified the logic based on @dcherian comments but in order to keep the result deterministic needed to use list logic instead of set logic. I also kept the OrderedDict instead of going with the default dict as the built in ordering methods as of py 3.6 were still insufficient for keeping the ordering consistent (I needed to pop FIFO) which doesn't seem possible until py 3.8. 2. I did add a failing test to capture the cases @kmuehlbauer pointed out.

I'm not sure I have my head wrapped around xarray enough to address @dcherian's latest comments though which is why i'm sharing the code at this point. All tests are passing except the new cases which were pointed out.

I'll try to continue to get time to update this but wanted to at least provide this status update at this point as its been awhile.

@kmuehlbauer @dcherian @shoyer If it would be easier it could abandon this PR and resubmit a new one as the code has drastically changed since the original comments were provided? Essentially I'm waiting for feedback or approval of this PR.

Hi, I've provided a new update to this PR (sorry it took me awhile both to get more familiar with the code and find the time to update the PR). I improved the logic to be a bit more performant and handle more edge cases as well as updated the test suite. I have a few questions:

1. The tests I wrote are a bit more verbose than the tests previously. I can tighten them down but I found it was easier for me to read the logic in this form. Please let me know what you prefer.
2. I'm still not quite sure I've captured all the scenarios as I'm a pretty basic xarray user so please let me know if there is still something I'm missing.

Ok, I'll work on extending the updates with the feedback and additional tests. Thanks!

Ok got it, I'll take a look and spin up a PR.
Thanks

I just ran in to this issue. While the previous fix seems to handle one case it doesn't handle all the cases. Before I clean this up and open a new PR does this look like its on the right track (it worked for my issue where I was concating multiple datasets which always had the same dims and coordinates but sometimes were missing variables)?

starts at line 353 on concat.py for k in datasets[0].variables: if k in concat_over: try: #new code for ds in datasets: if k not in ds.variables: #make a new array with the same dimensions and coordinates #by default this will be initialized to np.nan which is what we want from .dataarray import DataArray new_array = DataArray(coords=ds.coords, dims=ds.dims) ds[k] = new_array #end new code vars = ensure_common_dims([ds.variables[k] for ds in datasets]) except KeyError: #this can likely be removed then raise ValueError("%r is not present in all datasets." % k) combined = concat_vars(vars, dim, positions) assert isinstance(combined, Variable) result_vars[k] = combined