numpy icon indicating copy to clipboard operation
numpy copied to clipboard

DOC: Docs point to incorrect reduce for ma.alltrue and ma.sometrue

Open bmwoodruff opened this issue 1 year ago • 8 comments

Issue with current documentation:

When you run help(np.ma.alltrue) or help(np.ma.sometrue), you get

Help on method reduce in module numpy.ma.core:

reduce(target, axis=0, dtype=None) method of numpy.ma.core._MaskedBinaryOperation instance
    Reduce `target` along the given `axis`.

When you run help on any of the other 50+ ufuncs in the ma module, you get an appropriate doc message. For example help(np.ma.logical_and) yields (the first few lines):

Help on _MaskedBinaryOperation in module numpy.ma.core:

logical_and = <numpy.ma.core._MaskedBinaryOperation object>
    logical_and(x1, x2, /, out=None, *, where=True, casting='same_kind', order='K', dtype=None, subok=True[, signature])
    
    Compute the truth value of x1 AND x2 element-wise.
... it continues....

The issue seems to be the .reduce that appears on just these two functions (see below). https://github.com/numpy/numpy/blob/98e86d52ed79eb8810960bdcfac11271fc6c5434/numpy/ma/core.py#L1282-L1286

One consequence is that the attributes are different, with ma.alltrue and ma.sometrue being recognized as routines (while none of the others are).

>>>[getattr(np.ma,'logical_and'),getattr(np.ma,'alltrue')]
[<numpy.ma.core._MaskedBinaryOperation at 0x7e0545aa66d0>,
 <bound method _MaskedBinaryOperation.reduce of <numpy.ma.core._MaskedBinaryOperation object at 0x7e0545aa6750>>]
>>>[inspect.isroutine(np.ma.logical_and),inspect.isroutine(np.ma.alltrue)]
[False, True]

I'm hoping to get a script put into the CI build (thanks to @ngoldbaum 's suggestion) that checks to make sure new routines are always appropriately tagged in a .rst file so they get documented on the web.

Idea or request for content:

I have a couple questions:

  • How can we handle the ending .reduce at the of the definitions of ma.alltrue and ma.sometrue in a way that properly tags both the same as the rest of the ufuncs? Is that possible (or even wanted).
  • Do we want any of these ufuncs showing up in the web documentation as routines of the ma module? They all appear already with help(), but don't appear online.

bmwoodruff avatar May 30 '24 18:05 bmwoodruff

I think all you need to do is set __name__ manually for the ones that are actually reductions:

>>> print(pydoc.render_doc(np.ma.alltrue))
Python Library Documentation: method reduce in module numpy.ma.core

reduce(target, axis=0, dtype=None) method of numpy.ma.core._MaskedBinaryOperation instance
    Reduce `target` along the given `axis`.

>>> np.ma.alltrue.__func__.__name__ = "alltrue"
>>> print(pydoc.render_doc(np.ma.alltrue))
Python Library Documentation: method alltrue in module numpy.ma.core

alltrue(target, axis=0, dtype=None) method of numpy.ma.core._MaskedBinaryOperation instance
    Reduce `target` along the given `axis`.

It's __func__.__name__ because this is a bound method.

ngoldbaum avatar May 31 '24 17:05 ngoldbaum

I can help to open a PR on this

gangula-karthik avatar Jun 16 '24 06:06 gangula-karthik

@ngoldbaum I tried the fix you suggested right after you gave it, but all it did was update the name, with the help message still pointing to reduce. The PR submitted by @gangula-karthik shows what happens.

I tried to set the __doc__ attribute directly, but got

AttributeError: attribute 'doc' of 'method' objects is not writable

GTP-4o helped me see why this was a problem. I wasn't sure how to appropriately update the documentation on a ufunc that has a .reduce after it.

I think the appropriate documentation would be to provide the docstring of umath.logical_and for ma.alltrue, though this misses the fact that .reduce has been applied (but at least shows more relevant information). Another option could be to add separate docstrings for these functions, which mention that ma.alltrue is an alias for umath.logical_and followed by .reduce. Not sure which is desired.

bmwoodruff avatar Jun 17 '24 15:06 bmwoodruff

I tried

>>> np.ma.alltrue.__func__.__name__ = "alltrue"
>>> np.ma.alltrue.__func__.__doc__= np.logical_and.__doc__
>>> np.ma.sometrue.__func__.__name__ = "sometrue"
>>> np.ma.sometrue.__func__.__doc__= np.logical_or.__doc__
>>> help(np.ma.alltrue)
Help on method sometrue in module numpy.ma.core:

sometrue(target, axis=0, dtype=None) method of numpy.ma.core._MaskedBinaryOperation instance
...

What I suspected would happen, did. The two items np.ma.alltrue.__func__ and np.ma.sometrue.__func__ are the same item (which @gangula-karthik 's PR helped me see). So the second assignment is the only one saved.

bmwoodruff avatar Jun 17 '24 16:06 bmwoodruff

# dynamically set __name__ and __doc__ attributes
def set_name_and_doc(name, doc):
    def decorator(func):
        func.__name__ = name
        func.__doc__ = doc
        return func
    return decorator

logical_and = core._MaskedBinaryOperation(umath.logical_and)
logical_or = core._MaskedBinaryOperation(umath.logical_or)

@set_name_and_doc("alltrue", umath.logical_and.__doc__)
def alltrue(target, axis=0, dtype=None):
    return logical_and.reduce(target, axis=axis, dtype=dtype)

@set_name_and_doc("sometrue", umath.logical_or.__doc__)
def sometrue(target, axis=0, dtype=None):
    return logical_or.reduce(target, axis=axis, dtype=dtype)

np.ma.alltrue = alltrue
np.ma.sometrue = sometrue

print(help(np.ma.alltrue))
print(help(np.ma.sometrue))

Thanks for the comments, I was trying out some stuff and found that this solution which works:

>>> print(pydoc.render_doc(np.ma.alltrue))
Python Library Documentation: function alltrue in module __main__

alltrue(target, axis=0, dtype=None)
    logical_and(x1, x2, /, out=None, *, where=True, casting='same_kind', order='K', dtype=None, subok=True[, signature])
    
>>> print(pydoc.render_doc(np.ma.sometrue))
Python Library Documentation: function sometrue in module __main__

sometrue(target, axis=0, dtype=None)
    logical_or(x1, x2, /, out=None, *, where=True, casting='same_kind', order='K', dtype=None, subok=True[, signature])

The decorator approach creates independent function objects with correctly set attributes which avoids any conflicts. Not sure if there is already another decorator object existing, do let me know so that I can tweak the fix accordingly. Also appreciate any comments on this 😃

gangula-karthik avatar Jun 18 '24 15:06 gangula-karthik

I'm in uncharted (for me) territory here. If this works, and fixes up the docs for these two functions, then hooray.

I think this issue may be related to why the (some|all)true block of code exists in conf.py: https://github.com/numpy/numpy/blob/b8d1012a919910194f8b0ab761fc8ee4a17705ce/doc/source/conf.py#L439-L442

The function bitwise_not has a slightly different issue with it, which is on my "todo" list, but taken a back burner. Some of the last change to these portions of code happened 18 years ago, so I got to have a deep dive into git log.

bmwoodruff avatar Jun 18 '24 18:06 bmwoodruff

@gangula-karthik that seems reasonable to me, if you can try sending in a PR that would be great. Also maybe a test to make sure that __doc__ and __name__ are correct for everything in NumPy's public API. The public API is now fully specified as of NumPy 2.0, see numpy/tests/test_public_api.py for more tests related to validating NumPy's public Python API.

ngoldbaum avatar Jun 18 '24 18:06 ngoldbaum

@ngoldbaum @bmwoodruff, I have updated the code accordingly to fix the error. Now I'm working on the test case to check the __doc__ and __name__.

gangula-karthik avatar Jun 30 '24 14:06 gangula-karthik