issue_comments: 822107419

This data as json

html_url issue_url id node_id user created_at updated_at author_association body reactions performed_via_github_app issue
https://github.com/pydata/xarray/pull/5045#issuecomment-822107419 https://api.github.com/repos/pydata/xarray/issues/5045 822107419 MDEyOklzc3VlQ29tbWVudDgyMjEwNzQxOQ== 1217238 2021-04-19T01:21:22Z 2021-04-19T01:21:22Z MEMBER

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 . 

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   833778859