issue_comments

adding a new variable currently errors if we don't provide the append_dim argument:

Is this scenario now covered by the tests? Sorry if the answer is obvious; it's hard for me to discern just by looking at the code.

Thanks @shoyer for your more careful review of this PR. As usual you pick up on all the important details.

@shikharsg - are the issues you found in https://github.com/pydata/xarray/pull/2706#issuecomment-498194520 now resolved and covered by tests?

@jendrikjoe - thanks for digging in and finding this important issue!

This PR has been hanging around for a long time. (A lot of that is on me!) It would be good to get something merged soon. Here's what I propose. - Identify which datatypes can easily be appended now (e.g. floats, etc.) and which cannot (variable length strings) - Raise an error if append is called on the incompatible datatypes - Move forward with this PR, which is otherwise very nearly ready - Open a new issue to keep track of the outstanding incompatible types, which require upstream resolution in zarr

How does that sound to everyone?

Let’s make sure this new scenario is covered by tests!

Sent from my iPhone

On Jun 3, 2019, at 6:40 AM, Jendrik Jördening notifications@github.com wrote:

Gave you the permissions @shikharsg

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Hi @davidbrochart. I'm really sorry it takes me so long between reviews of your PR. It is very important work, and I appreciate your continued patience.

I looked at your new code, and I noticed that chunk_dim does not appear in the tests. I think it is important to test this parameter and verify that it works as expected. (This would also help me understand how it works, since it's not totally clear from the docs.)

Where do we stand on this PR? @davidbrochart - do you feel this is ready for a final review? Or do you want advice or feedback on anything?

@davidbrochart I would personally be happy to see anyone work on this. I'm sure @jendrikjoe would not mind if we make it a team effort!

We should definitely always make sure that we write data consistently (e.g., for dates), but checking for alignment of all coordinates could be expensive/slow.

This implies we should be checking for attributes compatibility before calling zarr.append.

So the problem in @davidbrochart's example is that there are different encodings on the time variables in the two datasets.

When writing datetimes, xarray automatically picks an encoding (i.e. days since 2000-01-01 00:00:00) based on some heuristics. When serializing the dataset, this encoding is used to encode the datetime64[ns] dtype into a different dtype, and the encoding is placed in the attributes of the store. When you open the dataset, the encoding is automatically decoded according to CF conventions. This can be disabled by using decode_cf=False or decode_times=False when you open the dataset.

In this case, xarray's heuristics are picking different encodings for the two dates. You could make this example work by manually specifying encoding on the appended dataset to be the same as the original.

This example illustrates the need for some sort of compatibility checks between the target dataset and the appended dataset. For example, checking for attribute compatibility would have caught this error.

Ok, with the example, I can see a bit better how this works.

Here is my main concern: there doesn't appear to be any alignment checking between the target dataset and the new data. The only check that happens is whether a variable with the same name already exists in the target store, if so, append is used (rather than creating a new array). What if the coordinates differ? What if the attributes differ?

I'm not sure this is a deal-breaker. But we should be very clear about this in the docs.

Hi @jendrikjoe -- thanks for submitting a PR to address one of the most important issues in xarray (IMHO)! I am very excited about your contribution and am looking forward to getting this feature merged.

I have many questions about how this works. I think the best way to move forward is to wait until we have a test for the append feature which involves the following steps: - Write a dataset to a zarr store - Open the store in append mode - Append data along a particular dimension

Seeing the code that accomplishes this will help clarify for me what is happening.

Thanks again for your contribution, and welcome to xarray!