issue_comments: 363072151

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/issues/1388#issuecomment-363072151 https://api.github.com/repos/pydata/xarray/issues/1388 363072151 MDEyOklzc3VlQ29tbWVudDM2MzA3MjE1MQ== 244887 2018-02-05T12:35:10Z 2018-02-05T12:35:10Z CONTRIBUTOR

@fujiisoup and @shoyer

Really enlightening comments above. I think I am starting to get the dao of xarray a bit better :)

I was thinking whether such aggregation methods (including argmin) should propagate the coordinate.

Agreed it would be nice to have a consistent and well reasoned rule for coordinate propagation in aggregation methods. I think a key point here, which gets brought up in your example is that it might make sense to have different subrules depending on the semantics of the operation. Functions like argmax are explicitly concerned with underlying "indices" (dimensions or otherwise) and so may call for different behavior from the mean, which explicitly is explicitly invariant under permutations of the underlying indices. The max/min/median functions are interesting case to think about, in that they are also invariant with under change of underlying indices, but can have potentially more than one index that they are associated with and do not "destroy" information about the value at those indices.

My concern with adding an additional dimension is that it is always a little surprising and error-prone when we invent new dimension names not supplied by the user (for example, this can lead to conflicting names)

Yeah, I felt a little dirty appending '_argmax'.

I think my favorite option is (2) with da.argmin_indices() returning a Dataset, which will allow da[da.argmin_indices()]

OK. I think I understand now why @fujiisoup proposed output a Dataset rather than an array. That's a natural syntax for getting the values from the indices.

Either way, I would like a separate dedicated method for returning multiple indexing arrays.

+1 to dedicated adding more methods if needed, since I think even if it isn;t needed the associated docs will need to make sure users are aware of the analogous idx* methods if they get added.

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