Skip to content

DOC: Prepend ma. to references in numpy.ma #16736

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

Merged
merged 3 commits into from
Nov 5, 2020
Merged

Conversation

takanori-pskq
Copy link

Related #13114 : separated from #16300 for further discussion

@takanori-pskq
Copy link
Author

takanori-pskq commented Jul 3, 2020

Instead of just copying docstrings from numpy versions, how about writing the docstrings of the functions in numpy.ma like "This function is numpy.ma version of the function foo. See numpy.foo for details"?

I tend to agree in principle, but I don't think that this is a general solution. While some functions in ma simply use the docstring from their np counterpart , some don't. There are examples of both patterns (and others) in numpy/ma/core.py for example. I am not familiar enough with np.ma to say whether an approach like this would scale well.

(from the conversation in #16300 )

@rossbar It's enough to replace to "This function is ..." only when the function simply copies the docstring in np version. If the function doesn't copy (the function has proper docstring apart from the np version), we don't have to change.

numpy/ma/core.py Outdated
@@ -5605,7 +5605,7 @@ def sort(self, axis=-1, kind=None, order=None,
--------
numpy.ndarray.sort : Method to sort an array in-place.
argsort : Indirect sort.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add prefix too.

numpy/ma/core.py Outdated
@@ -5605,7 +5605,7 @@ def sort(self, axis=-1, kind=None, order=None,
--------
numpy.ndarray.sort : Method to sort an array in-place.
argsort : Indirect sort.
lexsort : Indirect stable sort on multiple keys.
numpy.lexsort : Indirect stable sort on multiple keys.
searchsorted : Find elements in a sorted array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add prefix too.

@@ -3148,8 +3148,8 @@ def size(a, axis=None):
See Also
--------
shape : dimensions of array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be numpy.ma.shape.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be numpy.ma.shape.

This is the docstring for np.size, so it should be referencing functions from the numpy namespace, not np.ma I think.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Qiyu8 the reason I suggested @takanori-pskq split this off from #16300 was to see if we could find a way to make the links work in np.ma without having to prepend numpy to everything, so as not to clutter up the docstring for the numpy functions.

Perhaps it's worth looking at the getdoc method of the _frommethod and/or _convert2ma classes? Again, I'm not too familiar with the details of how np.ma is documented, so please share any ideas. I just think it's worth exploring if we can fix some of these broken links in np.ma without having to touch the docstrings for the corresponding np objects.

@@ -3148,8 +3148,8 @@ def size(a, axis=None):
See Also
--------
shape : dimensions of array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be numpy.ma.shape.

This is the docstring for np.size, so it should be referencing functions from the numpy namespace, not np.ma I think.

@takanori-pskq
Copy link
Author

Some functions use neither _frommethod nor _convert2ma, and just copy (foo.__doc__ = np.foo.__doc__). If _convert2ma can be applied for these functions, it is enough to see the getdoc method in _convert2ma, and add the process to prepend numpy. to the links in the docstring which start other than numpy..

@rossbar
Copy link
Contributor

rossbar commented Jul 6, 2020

If _convert2ma can be applied for these functions, it is enough to see the getdoc method in _convert2ma, and add the process to prepend numpy. to the links in the docstring which start other than numpy.

This would work, though then you'd be automatically overriding any references to np.ma versions of functions when they're defined. Take np.ma.var for instance: there are six references in the See Also section, five of which are references to functions. Of those, two (std and mean) have np.ma counterparts, whereas the other three do not. If you prepend numpy to everything, than you lose the specific references to the ma versions of mean and std. Since the docstrings appear to be simply copied over from the non-masked versions, it doesn't seem like you'd lose anything, but in the case where the ma version of a function has an ma-specific docstring, you'd be overriding the reference to it.

@mattip
Copy link
Member

mattip commented Oct 6, 2020

What is the status of this experiment?

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping @mattip

tl;dr - upside: fixes some broken links | downside: makes some See Also sections uglier and in some cases redirects away from ma functions when they would be a better fit.

I'm still ambivalent about this. See the proposed changes to the See Also section of var. Currently, the ma.var docs link to the ma versions of std and mean in the See Also section: https://numpy.org/devdocs/reference/generated/numpy.ma.var.html. With this change, those links will instead point to the numpy counterparts, rather than numpy.ma. This also clutters-up the See Also sections in the core functions, as prepending numpy. is not necessary to make those links work.

The real problem stems from how ma docs are generated. Ideally there would be a solution that would add e.g. .. currentmodule's etc. where appropriate when the ma docs are being generated, but there doesn't seem to be a one-size-fits-all solution.

There seems to be a nice push to get #13114 closed (big +1 IMO), so I'm okay accepting this as-is to further that goal and revisiting to fix the details later on.

@mattip
Copy link
Member

mattip commented Oct 6, 2020

Ideally there would be a solution that would add e.g. .. currentmodule's etc. where appropriate when the ma docs are being generated

Could this be done with a custom template option in the autosummary in doc/source/referencemaskedarray.baseclass.rst?

@takanori-pskq
Copy link
Author

I'll try using template.

@takanori-pskq
Copy link
Author

Now I work to fix this problem in #17515 .

@takanori-pskq
Copy link
Author

Modified to prepend ma. instead of numpy.. (see #17515 (comment)).

@takanori-pskq takanori-pskq changed the title DOC: Prepend numpy. to references in numpy.ma DOC: Prepend ma. to references in numpy.ma Nov 4, 2020
Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the template works as expected.

@mattip mattip merged commit 93d3e7c into numpy:master Nov 5, 2020
@mattip
Copy link
Member

mattip commented Nov 5, 2020

Thanks @takanori-pskq.

@takanori-pskq takanori-pskq deleted the i13114-2 branch November 5, 2020 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants