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

DOC: Add and fixup/move docs for descriptor changes #25946

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

seberg
Copy link
Member

@seberg seberg commented Mar 6, 2024

This is a "pre" PR for the actual changes in #25943. The point is that the PR breaks the docs build as it is enough of an ABI break that downstream has to recompile.

That will take a few days (maybe a bit longer if pyibind11 proofs problematic, although it seems problematic use of pybind11 is rare, e.g. in SciPy only one file/module was affected).

Thus, split it out so we can put it in (and also see doc mistakes).

The ``PyArray_Descr`` struct has been changed
---------------------------------------------
One of the most impactful C-API changes is that the ``PyArray_Descr`` struct
is no more opaque to allow us to add additional flags and have
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say it is now opaque instead of no longer opaque? Unless I am misinterpreting the word opaque, but I would assume you want to make it more opaque to allow changing it later without breaking user code again

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, it was meant to be a "now", not a "no". It is mostly changed really, but I think it makes sense to write it as opaque.
(I.e. elsize is available if you set -DNPY_TARGET_VERSION=NPY_2_0_API_VERSION, but that should be relevant only for authors of new-style DTypes.)

@seberg seberg force-pushed the descr-abi-break-docs branch 3 times, most recently from fcbbab1 to 0f987a8 Compare March 6, 2024 08:16
@rgommers
Copy link
Member

rgommers commented Mar 6, 2024

Looks like a good idea to split this off, to keep PR sizes reasonable. This already accumulated a conflict in c-api/array.rst.

[skip actions] [skip cirrus] [skip azp]
@seberg
Copy link
Member Author

seberg commented Mar 6, 2024

This already accumulated a conflict in c-api/array.rst.

Fixed. The important thing is I would like this merged before the other PR. Because the other PR will break doc builds (hopefully not for long), and we need the newest docs visible.

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.

LGTM. One typo that became a refactor.

Co-authored-by: Matti Picus <matti.picus@gmail.com>
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.

Now that I looked at the actual rendered documentation, I see some problems


Where not possible, new accessor functions are required:
* ``PyDataType_ELSIZE`` and ``PyDataType_SET_ELSIZE`` (note that the result
is now ``npy_intp`` and not ``int``).
Copy link
Member

Choose a reason for hiding this comment

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

Formatting is off in the rendered page

Suggested change
is now ``npy_intp`` and not ``int``).
is now ``npy_intp`` and not ``int``).


**Custom User DTypes:**
Existing user dtypes must now use ``PyArray_DescrProto`` to define their
dtype and slightly modify the code. See note in `PyArray_RegisterDataType`.
Copy link
Member

Choose a reason for hiding this comment

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

Link to PyArray_RegisterDataType is broken in the rendered page.

is non- ``NULL`` (the fields member of the base descriptor can be
non- ``NULL`` however).

.. c:type:: PyArray_ArrayDescr
Copy link
Member

Choose a reason for hiding this comment

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

Is this indented on purpose? See the rendered page

Some dtypes have additional members which are accessible through
`PyDataType_NAMES`, `PyDataType_FIELDS`, `PyDataType_SUBARRAY`, and
in some cases (times) `PyDataType_C_METADATA`.

Copy link
Member

Choose a reason for hiding this comment

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

These reference links are not working. Do you need a domain?

See `PyDataType_ELSIZE` and `PyDataType_SET_ELSIZE` for a way to access
this field in a NumPy 1.x compatible way.

.. c:member:: npy_intp alignment
Copy link
Member

Choose a reason for hiding this comment

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

The links are broken. Do you need a domain?

An ordered tuple of field names. It is NULL if no field is
defined.
See `PyDataType_ALIGNMENT` for a way to access this field in a NumPy 1.x
compatible way.
Copy link
Member

Choose a reason for hiding this comment

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

The link is broken.

----------------------------------------------
Unless compiling only with NumPy 2 support, the ``elsize`` and ``aligment``
fields must now be accessed via `PyDataType_ELSIZE`,
`PyDataType_SET_ELSIZE`, and `PyDataType_ALIGNMENT`.
Copy link
Member

Choose a reason for hiding this comment

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

The links above are broken. Maybe you can fix the other warnings in the release notes, see the warnings starting here

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

spotted a couple typos

One of the most impactful C-API changes is that the ``PyArray_Descr`` struct
is now more opaque to allow us to add additional flags and have
itemsizes not limited by the size of ``int`` as well as allow improving
structured dtypes in the future and not burdon new dtypes with their fields.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
structured dtypes in the future and not burdon new dtypes with their fields.
structured dtypes in the future and not burden new dtypes with their fields.

structured dtypes in the future and not burdon new dtypes with their fields.

Code which only uses the type number and other initial fields is unaffected.
Most code will hopefull mainly access the ``->elsize`` field, when the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Most code will hopefull mainly access the ``->elsize`` field, when the
Most code will hopefully mainly access the ``->elsize`` field, when the

@mattip
Copy link
Member

mattip commented Mar 6, 2024

Lets merge this now and work on cleanups later.

@mattip mattip merged commit ae8f784 into numpy:main Mar 6, 2024
63 checks passed
@seberg
Copy link
Member Author

seberg commented Mar 6, 2024

Thanks, one additional small fixup, will be to add the additional reserved_null field, we discussed.

jorisvandenbossche added a commit to apache/arrow that referenced this pull request Mar 13, 2024
…elsize (#40418)

### Rationale for this change

NumPy 2.0 is changing some ABI, see the issue description and numpy/numpy#25946 for more details. 

The changes here should make our code compatible both with current numpy 1.x and future numpy 2.x

* GitHub Issue: #40376

Lead-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Sebastian Berg <sebastianb@nvidia.com>
Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
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