DOC: Fix bad reference in ``numpy.ma`` by takanori-pskq · Pull Request #16300 · numpy/numpy

takanori-pskq · 2020-05-19T10:09:36Z

Partly fixes #13114

charris · 2020-05-20T22:01:20Z

close/reopen

rossbar · 2020-05-27T19:35:34Z

Quick question @takanori-pskq - you've marked this as WIP: does that mean that it is not ready for review in your opinion?

Also note that you don't have to fix all of the issues in #13114 in one big PR - in fact it would be preferable (from a review standpoint) if changes were broken up into smaller PRs that deal with coherent portions of the documentation. It seems this is how you originally intended to organize this PR (limiting changes to numpy.ma), but 9f38b49 introduces a large change to the scalar types docs.

takanori-pskq · 2020-05-28T02:23:07Z

@rossbar I marked as WIP because the build fails (that will be resolved by PRs #16296, #16311). And your comment about the commit 9f38b49 is right. I'll revert and create another PR.

rossbar · 2020-06-12T00:54:18Z

@takanori-pskq now that #16296 and #16311 are merged, do you want to try to rebase on master and see if that fixes the CI? I'd be happy to take a look afterwards!

rossbar

Thanks for taking a stab at this @takanori-pskq , this is certainly an improvement in fixing up the broken links in np.ma: I recorded 806 before this change (on 19f91f) and 738 afterwards. I think many of the changes are non-controversial. However, there is one pattern where numpy. is prepended to functions in the numpy docstrings so that links in the corresponding numpy.ma docstrings resolve to the numpy versions. This is okay, but has the unfortunate side-effect of cluttering up the original numpy docstrings a bit. Do you think there might be another way to get those links resolved using a different approach?

This PR touches a lot of different things so it will take me few passes before I can wrap my head around everything - I hope you can bear with me!

takanori-pskq · 2020-06-30T14:29:04Z

@rossbar 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"?

rossbar · 2020-07-01T22:48:13Z

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.

Nevertheless, there are some changes in this PR that do indeed correct some broken links without affecting existing docstrings in other namespaces. Perhaps we could separate those out from the others that might need a more detailed discussion? If you're interested in doing so I'd be happy to help try to ID the straight-forward changes.

takanori-pskq · 2020-07-02T01:54:17Z

I'll separate the changes prepending numpy. (the lines you commented). Is there any changes that should be separated?

rossbar · 2020-07-02T20:22:07Z

I'll separate the changes prepending numpy. (the lines you commented). Is there any changes that should be separated?

I think that's a good place to start. The addition of the missing functions to the routines doc and the corresponding <module>.__all__ should certainly stay in.

rossbar

LGTM - fixes about 25 broken xrefs in the ma docs (794 link warnings before, 769 after this change).

mattip · 2020-07-04T19:41:28Z

It seems this exposes new functions, should they have tests (maybe they already do)?

rossbar · 2020-07-04T20:58:55Z

It seems this exposes new functions, should they have tests (maybe they already do)?

Good point - AFAICT, the related creation functions that are already exposed in np.ma (e.g. np.ma.zeros) aren't explicitly tested in numpy/ma/tests. Maybe this is an oversight that should be remedied? I'm not sure what the policy is for testing the ma functions that are derived from their non-masked counterparts (i.e. the functions like ones_like etc., which are defined in numpy/ma/core.py via the _convert2ma class).

takanori-pskq · 2020-07-15T14:43:13Z

rebased, and added tests for some functions.

rossbar

A couple comments about the tests that have been added, but everything else LGTM

takanori-pskq · 2020-08-06T06:51:21Z

rebased and fixed the tests.

mattip · 2020-08-10T07:30:11Z

There are a few comments open about shuffling tests around. @takanori-pskq would you like to relate to them or should we put this in as-is and somehow mark that for a later refactoring?

rossbar

A couple suggested cleanups to the tests but otherwise LGTM!

Subsequent changes and discussion since initial approval

rossbar

As it stands, the newly-exposed zeros_like, ones_like, and full_like functions won't support some of the masked array parameters (e.g. fill_value, hardmask). I think this is okay as empty_like, which is already exposed, doesn't either. I'm fine with following the precedent set by empty_like and leaving any potential updates to parameter support for the *_like arguments for a future PR.

Everything else LGTM, thanks @takanori-pskq for taking the time to re-organize the tests.

rossbar

I was looking at this a bit more closely and just noticed an unfortunate name collision in the argument names of np.ma.full: fill_value is the name for both the value that full uses to fill the array, and the fill_value of the mask. In combination with _convert2ma, this can result in nasty errors:

>>> np.ma.full(3, fill_value=5)
Traceback (most recent call last)
   ...
TypeError: full() missing 1 required positional argument: 'fill_value'

There might be a way around this using position-only arguments, but unfortunately I don't think the _convert2ma machinery is sufficient to handle this, at least not in it's current form.

Parameter name collision between non-ma and ma versions of full

mattip · 2020-10-06T08:44:51Z

Can we merge this PR as-is and continue cleanup afterwards or is there still work that needs to be done here?

When we merge it, we should re-open gh-13114 since this is marked as closing it, or is there a way to tell github not to close it on merge

eric-wieser · 2020-10-06T09:01:12Z

I think @rossbar's concerns are valid - this PR mixes two things:

DOC: Fixing references in docstrings
ENH: Add new numpy.ma.* aliases for numpy.* functions

The second point needs more work, and looks problematic in its current form.

BvB93 · 2020-10-07T14:42:33Z

FYI, the the new np.ma.* aliases have been moved to #17480.

rossbar

Looks good: 410 -> 397 warnings. Thanks @takanori-pskq !

charris changed the title ~~DOC: fix bad reference in numpy.ma~~ DOC: Fix bad reference in numpy.ma May 19, 2020

charris added 03 - Maintenance 04 - Documentation component: numpy.ma masked arrays labels May 19, 2020

charris closed this May 20, 2020

charris reopened this May 20, 2020

takanori-pskq changed the title ~~DOC: Fix bad reference in numpy.ma~~ WIP, DOC: Fix bad reference in numpy.ma May 23, 2020

takanori-pskq changed the title ~~WIP, DOC: Fix bad reference in numpy.ma~~ DOC: Fix bad reference in numpy.ma Jun 11, 2020

takanori-pskq force-pushed the wb branch from 02ff690 to 4414305 Compare June 12, 2020 03:03

rossbar self-requested a review June 30, 2020 03:21

rossbar reviewed Jun 30, 2020

View reviewed changes

Comment thread numpy/core/_add_newdocs.py Outdated

Comment thread numpy/core/_add_newdocs.py Outdated

Comment thread numpy/core/fromnumeric.py Outdated

Comment thread numpy/core/fromnumeric.py Outdated

Comment thread numpy/core/fromnumeric.py Outdated

takanori-pskq mentioned this pull request Jul 3, 2020

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

Merged

rossbar previously approved these changes Jul 4, 2020

View reviewed changes

takanori-pskq force-pushed the wb branch from ce257ec to c565ee9 Compare July 15, 2020 11:38

rossbar self-requested a review July 29, 2020 20:36

rossbar reviewed Aug 4, 2020

View reviewed changes

Comment thread numpy/ma/tests/test_core.py Outdated

Comment thread numpy/ma/tests/test_extras.py Outdated

takanori-pskq force-pushed the wb branch from 5a74766 to 2e1c766 Compare August 6, 2020 06:40

rossbar reviewed Aug 10, 2020

View reviewed changes

Comment thread numpy/ma/tests/test_extras.py Outdated

Comment thread numpy/ma/tests/test_extras.py Outdated

Comment thread numpy/ma/tests/test_extras.py Outdated

rossbar previously approved these changes Aug 12, 2020

View reviewed changes

rossbar reviewed Aug 12, 2020

View reviewed changes

eric-wieser reviewed Oct 6, 2020

View reviewed changes

Comment thread numpy/ma/core.py Outdated

DOC: Fixing references in docstrings

59fa6ac

takanori-pskq force-pushed the wb branch from 1641aea to 59fa6ac Compare October 7, 2020 06:08

takanori-pskq mentioned this pull request Oct 7, 2020

ENH: Add new numpy.ma.* aliases #17480

Closed

rossbar approved these changes Oct 7, 2020

View reviewed changes

rossbar merged commit 2a267e6 into numpy:master Oct 7, 2020

takanori-pskq deleted the wb branch October 8, 2020 00:47

takanori-pskq mentioned this pull request Oct 8, 2020

ENH: Add ma.full and ma.full_like #17500

Closed

Conversation

takanori-pskq commented May 19, 2020

Uh oh!

charris commented May 20, 2020

Uh oh!

rossbar commented May 27, 2020

Uh oh!

takanori-pskq commented May 28, 2020

Uh oh!

rossbar commented Jun 12, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

takanori-pskq commented Jun 30, 2020

Uh oh!

rossbar commented Jul 1, 2020

Uh oh!

takanori-pskq commented Jul 2, 2020

Uh oh!

rossbar commented Jul 2, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

mattip commented Jul 4, 2020

Uh oh!

rossbar commented Jul 4, 2020

Uh oh!

takanori-pskq commented Jul 15, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

takanori-pskq commented Aug 6, 2020

Uh oh!

mattip commented Aug 10, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

mattip commented Oct 6, 2020

Uh oh!

eric-wieser commented Oct 6, 2020

Uh oh!

Uh oh!

BvB93 commented Oct 7, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants