Docs would be great! Particularly if the current docs are out of date now. Thanks @matzegoebel

Thanks a lot @matzegoebel !

I've fixed the location of the whats-new note. Is this ready to go in?

Yes. I have one typing question but we can merge regardless if needed

I've fixed the location of the whats-new note. Is this ready to go in?

could you also resolve the merge conflicts? It seems we can't "approve and run" the CI with conflicts.

I'm not sure how to use this, because np.can_cast with unsafe casting mostly returns True, e.g. np.can_cast(str, float, casting="unsafe")==True. The error that is implemented in the tests would not be caught then.

Oh, good point, thanks for checking.

But I could explicitly try to cast the new values to the datatype of the original values with .astype. Would that be an option?

Yes, I like this idea!

I revised the pre-assignment checks. In my opinion xr.align is not so helpful when checking that the dimension sizes and coordinates are consistent, because it doesn't fail when the dimension size of the two Datasets is different, but the coordinate of the second Dataset is a subset of the first one.

Could you kindly elaborate on this issue, maybe with a specific example?

If, despite the checks, an error occurs during the assignment, e.g. due to a type error, and the dataset has been updated already partially, the user is informed about this.

np.can_cast with casting='unsafe can check this. It sounds like this would probably be something good to add to our checks :)

ok, I deleted the copy stuff and included a few checks to catch possible errors before setting the values. Did I miss anything? How do we check for "type errors that don't coerce", as you mentioned?

Excellent. Re the checks — I mostly meant that it was going to be very rare for something to get through — I don't think it's necessary to check for something like "type errors that don't coerce".

Which errors would __getitem__ miss? At least type errors that don't coerce; are there other cases?

The issue with a deep copy of the whole dataset is that it's very expensive. It's probably better to have that rather than nothing, but it could have confusing performance effects given that people are often going to be mutating values to reduce copies.

These aren't strongly held views though. Any thoughts from others?

Great, this is shaping up.

I think we can find a way of failing early on bad indexes without attempting the whole operation on a copy.

At the very least, we could call __getitem__ with the indexes and see whether that passes. There may be better ways yet.

I also think that because the currently proposed code uses a shallow copy, it may be mutating the original when bad indexes are passed — it's worth adding a test to confirm.

that's a flaky test which randomly fails (see #4539). You can safely ignore it, the CI should pass on the next run.

Strong +1 from me on narrowing scope whenever possible. Adding features incrementally is much easier than doing things all at once :)

On Sun, Apr 18, 2021 at 6:15 PM Maximilian Roos @.***> wrote:

@.**** commented on this pull request.

In xarray/core/dataset.py https://github.com/pydata/xarray/pull/5045#discussion_r615490870:

• loop over dataset variables

• for var, da in self.items():
• val = value
• if type(value) == xr.core.dataset.Dataset:
• val = value[var]

• only set value if all dimensions are present

• if all([k in da.dims for k in key.keys()]):
• da[key] = val

V good point. Thanks.

Either a) raising on missing dimensions or b) "entirely set to zero" in this case, would be reasonable imo.

To the extent you're less confident that (b) is correct, I'd suggest we move forward with (a) and evaluating whether we should do (b) separately.

(though @shoyer https://github.com/shoyer if you're up for it, let's discuss the general pace of reviews next week and whether you think this sort of "narrowing" of PR scope is a reasonable tactic)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/5045#discussion_r615490870, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJJFVQR6Q6VI44EEY2SF43TJN7ZTANCNFSM4ZKTDBUA .

@matzegoebel forgive the very long delay on the review. We're planning to find a better system to ensure these don't drop through.

I would be up for adding this, for consistency. I don't think I've ever needed the functionality, but it also doesn't make the interface more complicated given it's mirroring __getitem__.

We probably need to think through whether there are any corner cases here; I can't think of any atm.

Any other thoughts?