-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Clarify documentation of argmin/argmax #3264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Thanks for the issue @rdrussotto - would definitely take a PR for this (even if it's a unsophisticated copy-paste of a snippet of the numpy docs) |
Turns out this is more difficult than I thought... I did some digging into where these doc pages are defined (e.g. http://xarray.pydata.org/en/stable/generated/xarray.Dataset.argmin.html) and realized they are auto-generated from a template in core/ops.py, which is the same for various "reduce" functions, most of which are more self-explanatory like max or min. Creating more useful doc page for argmin/argmax would, I believe, require writing a new template for them, which would in turn require separating argmin and argmax from "NAN_REDUCE_METHODS" and writing a new "inject" function for them. This would mess up a lot of other places that use the "inject_reduce_methods" function. Not sure if this is worth the effort especially since @shoyer left a todo note saying he's going to rewrite this module anyway. It might still be worth adding an example use case of argmax/argmin, maybe finding the dates of the highest and lowest temperatures in the toy weather data example. |
Actually, maybe just an if statement in "inject_reduce_methods" conditioned on the name of the function would work for switching to a different template. I might give this a try later. |
@rdrussotto Since v0.16.0 (specifically bdcfab5), |
@sjvrijn The new documentation looks great, with examples illustrating how it works! idxmin/idxmax is also a great addition. I think all of the concerns I raised in this issue are resolved. |
The documentation pages only refer to argmin/argmax without defining them (e.g. "Returns: New DataArray object with argmin applied to its data"), forcing users to refer to the NumPy documentation to learn what the functions do do and try to guess at how the XArray version will work. I think the doc pages should say what these functions actually do, possibly with an example of a common use case, e.g. finding the latitude where some quantity is minimized.
Also, the first time I used argmin/argmax I wasn't sure from the documentation whether the returned value is the coordinate index or the raw index. I think this should be explicitly stated. I also like the idea mentioned in issue #1388 of creating an alternate version of argmin/argmax that does return the coordinate index, since it would make it easier to stay within the coordinate indices all the time.
The text was updated successfully, but these errors were encountered: