Skip to content
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

dtype missing docstrings since 1.20.1 #18740

Closed
bschnurr opened this issue Apr 7, 2021 · 7 comments · Fixed by #18775
Closed

dtype missing docstrings since 1.20.1 #18740

bschnurr opened this issue Apr 7, 2021 · 7 comments · Fixed by #18775

Comments

@bschnurr
Copy link

bschnurr commented Apr 7, 2021

Reproducing code example:

import numpy as np
print(np.dtype.__doc__)

Error message:

None

Expected:

dtype(obj, align=False, copy=False)

    Create a data type object.

    A numpy array is homogeneous, and contains elements described by a
    dtype object. A dtype object can be constructed from different
    combinations of fundamental numeric types.

    Parameters
    ----------
    obj
        Object to be converted to a data type object.
    align : bool, optional
        Add padding to the fields to match what a C compiler would output
        for a similar C-struct. Can be ``True`` only if `obj` is a dictionary
        or a comma-separated string. If a struct dtype is being created,
        this also sets a sticky alignment flag ``isalignedstruct``.
    copy : bool, optional
        Make a new copy of the data-type object. If ``False``, the result
        may just be a reference to a built-in data-type object.

    See also
    --------
    result_type

    Examples
    --------
    Using array-scalar type:

    >>> np.dtype(np.int16)
    dtype('int16')

    Structured type, one field name 'f1', containing int16:

    >>> np.dtype([('f1', np.int16)])
    dtype([('f1', '<i2')])

    Structured type, one field named 'f1', in itself containing a structured
    type with one field:

    >>> np.dtype([('f1', [('f1', np.int16)])])
    dtype([('f1', [('f1', '<i2')])])

    Structured type, two fields: the first field contains an unsigned int, the

    >>> np.dtype([('f1', np.uint64), ('f2', np.int32)])
    dtype([('f1', '<u8'), ('f2', '<i4')])

    Using array-protocol type strings:

    >>> np.dtype([('a','f8'),('b','S10')])
    dtype([('a', '<f8'), ('b', 'S10')])
    Using comma-separated field formats.  The shape is (2,3):

    >>> np.dtype("i4, (2,3)f8")
    dtype([('f0', '<i4'), ('f1', '<f8', (2, 3))])

    Using tuples.  ``int`` is a fixed type, 3 the field's shape.  ``void``

    >>> np.dtype([('hello',(np.int64,3)),('world',np.void,10)])
    dtype([('hello', '<i8', (3,)), ('world', 'V10')])

    Subdivide ``int16`` into 2 ``int8``'s, called x and y.  0 and 1 are
    the offsets in bytes:

    >>> np.dtype((np.int16, {'x':(np.int8,0), 'y':(np.int8,1)}))
    dtype((numpy.int16, [('x', 'i1'), ('y', 'i1')]))

    Using dictionaries.  Two fields named 'gender' and 'age':

    >>> np.dtype({'names':['gender','age'], 'formats':['S1',np.uint8]})
    dtype([('gender', 'S1'), ('age', 'u1')])

    Offsets in bytes, here 0 and 25:

    >>> np.dtype({'surname':('S25',0),'age':(np.uint8,25)})
    dtype([('surname', 'S25'), ('age', 'u1')])

NumPy/Python version information:

import sys, numpy; print(numpy.version, sys.version)
1.20.1 3.9.2 (tags/v3.9.2:1a79785, Feb 19 2021, 13:44:55) [MSC v.1928 64 bit (AMD64)]

@seberg
Copy link
Member

seberg commented Apr 7, 2021

Its a bug in arr_add_docstring here:

else if (Py_TYPE(obj) == &PyType_Type) {
PyTypeObject *new = (PyTypeObject *)obj;
_ADDDOC(new->tp_doc, new->tp_name);
}

That code can't cope with the fact that dtype is a metaclass instance now. Should be a simple fix.

@charris charris modified the milestones: 1.20.2 release, 1.20.3 release Apr 7, 2021
@charris
Copy link
Member

charris commented Apr 14, 2021

@seberg Did you make a fix for this?

@seberg
Copy link
Member

seberg commented Apr 14, 2021

no, not yet. EDIT: are you preparing another release already?

@seberg
Copy link
Member

seberg commented Apr 14, 2021

Hmm, I am trying to look at it, but it turns out a bit more complicated. Normally, tp_doc is read, but for some reason (even though np.dtype is not a heap type!) tp_doc is not being used...

Now, I can just set __doc__ as well, we could even always only use __doc__, which might even make more sense (I suppose this is probably only possible on "newer" Python, but it doesn't matter, that would also fix PyPy).
That however means that I have to set both __doc__ and __text_signature__. Which is fine.

Although, I now see that we never added the ")\n--\n\n" signature indicator to begin with, so just adding to __dict__ is maybe good for a backport.

seberg added a commit to seberg/numpy that referenced this issue Apr 14, 2021
This ensures that `help(np.dtype)` produces a result.  I am not
exactly sure why it picks up `__doc__` from the dict instead of
`tp_doc` right now. It probably is due to the combination of
inheritance and the fact that the dict always includes `None`
and gets preference during inheritance.
(That probably makes a lot of sense to not inherit the `type`
docstring by default.)

Modifying the dictionary directly is not really good style, either,
but hopefully works.

Closes numpygh-18740
@charris
Copy link
Member

charris commented May 6, 2021

Going to push this off to 1.22.0 for tracking. If there is a fix, we can backport to 1.21.x.

@WarrenWeckesser
Copy link
Member

+1 for a fix. dtype really needs a docstring! For example, if someone (:grin:) were to absentmindedly write np.dtype(np.datetime64, 's'), it might take them a while to figure out why creating arrays with this dtype was not working as expected. Then checking the arguments of dtype in ipython with np.dtype? would provide no help.

(This is also a case where I--I mean, someone--would prefer a language with stricter typing, where a bool type allows only True or False.)

seberg added a commit to seberg/numpy that referenced this issue Oct 4, 2021
This ensures that `help(np.dtype)` produces a result.  I am not
exactly sure why it picks up `__doc__` from the dict instead of
`tp_doc` right now. It probably is due to the combination of
inheritance and the fact that the dict always includes `None`
and gets preference during inheritance.
(That probably makes a lot of sense to not inherit the `type`
docstring by default.)

Modifying the dictionary directly is not really good style, either,
but hopefully works.

Closes numpygh-18740
@seberg
Copy link
Member

seberg commented Oct 4, 2021

If someone wants a fix, they may want to review gh-18775 ;)

prefer a language with stricter typing, where a bool type allows only True or False

I seem to recall that even Python enforces stricter typing for bools occasionally, so there might be arguments to be made in certain cases at least.

WarrenWeckesser pushed a commit that referenced this issue Oct 6, 2021
…18775)

* DOC: Ensure that we add documentation also as to the dict for types

This ensures that `help(np.dtype)` produces a result.  I am not
exactly sure why it picks up `__doc__` from the dict instead of
`tp_doc` right now. It probably is due to the combination of
inheritance and the fact that the dict always includes `None`
and gets preference during inheritance.
(That probably makes a lot of sense to not inherit the `type`
docstring by default.)

Modifying the dictionary directly is not really good style, either,
but hopefully works.

Closes gh-18740
charris pushed a commit to charris/numpy that referenced this issue Oct 10, 2021
This ensures that `help(np.dtype)` produces a result.  I am not
exactly sure why it picks up `__doc__` from the dict instead of
`tp_doc` right now. It probably is due to the combination of
inheritance and the fact that the dict always includes `None`
and gets preference during inheritance.
(That probably makes a lot of sense to not inherit the `type`
docstring by default.)

Modifying the dictionary directly is not really good style, either,
but hopefully works.

Closes numpygh-18740
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants