8000 MAINT: backport Cython API cleanup to 1.17.x, remove docs by rgommers · Pull Request #14593 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: backport Cython API cleanup to 1.17.x, remove docs #14593

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 19, 2019
Merged

MAINT: backport Cython API cleanup to 1.17.x, remove docs #14593

merged 4 commits into from
Nov 19, 2019

Conversation

rgommers
Copy link
Member

Backport of gh-14562 and removal of the docs on extending with Cython and Numba.

The Numba part says:
Both CTypes and CFFI allow the more complicated distributions to be used directly in Numba after compiling the file distributions.c into a DLL or so.
So that seems broken to and should be taken out.

@rgommers rgommers mentioned this pull request Sep 25, 2019
Closed
rgommers
rgommers commented Sep 25, 2019
View reviewed changes
doc/source/reference/random/extending.rst


Both CTypes and CFFI allow the more complicated distributions to be used
directly in Numba after compiling the file distributions.c into a DLL or so.
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I misread this. I think the Numba example above still works so perhaps we should leave that in. Compiling distributions.c doesn't work I think, so prefer to remove that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to have been broken in some of the reorganization due to the includes changing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I triple checked and it still is possible to build the DLL/so from the src dist. Not possible form the wheel though since there are no c files.

Copy link
Member

Choose a reason for hiding this comment

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

This document is deleted in the backport, since it is unclear and the examples don't work.

bashtage
bashtage reviewed Oct 16, 2019
View reviewed changes
Copy link
Contributor
@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Probably not important, but some of the removed lines are still true statements.

doc/source/reference/random/index.rst
@@ -151,11 +151,6 @@ What's New or Different
select distributions
* Optional ``out`` argument that allows existing arr 8000 ays to be filled for
select distributions
* All BitGenerators can produce doubles, uint64s and uint32s via CTypes
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement is still true on 1.17.x.

Copy link
Member

Choose a reason for hiding this comment

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

@rgommers Comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea, I have no reason to doubt @bashtage's comment

Copy link
Member

Choose a reason for hiding this comment

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

Should we omit the deletion here, or is reasonable for 1.17 and we will cover things better in 1.18? I'd like to put this backport in. Maybe have another if there are changes in master?

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to omit this deletion, will verify later today or tomorrow and change if it still works.

Copy link
Member

Choose a reason for hiding this comment

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

OK, will wait. @bashtage makes the same comment in several places.

Copy link
Member

Choose a reason for hiding this comment

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

@rgommers Still want to look at this?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, failed to make time - not going to happen until end of next week at the earliest. added the 1.17.4 milestone, it seems that it should go into that release

Copy link
Member

Choose a reason for hiding this comment

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

reverted the deletion: BitGenerator.ctypes still exists

doc/source/reference/random/new-or-different.rst
@@ -56,11 +56,6 @@ And in more detail:
``randn`` methods are only available through the legacy `~.RandomState`.
This replaces both ``randint`` and the deprecated ``random_integers``.
* The Box-Muller method used to produce NumPy's normals is no longer available.
* All bit generators can produce doubles, uint64s and
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment.

Copy link
Member

Choose a reason for hiding this comment

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

reverted

doc/source/reference/random/index.rst
* All BitGenerators can produce doubles, uint64s and uint32s via CTypes
(`~.PCG64.ctypes`) and CFFI (`~.PCG64.cffi`). This allows the bit generators
to be used in numba.
* The bit generators can be used in downstream projects via
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this statement is also True, since bit generator all export a capsule that can be unpacked in Cython get
the function pointers.

Copy link
Member

Choose a reason for hiding this comment

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

reverted the deletion

Copy link
Member

Choose a reason for hiding this comment

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

on second thought, I think that use is too obscure to be practical. Let's remove that from this release.

doc/source/reference/random/extending.rst


Both CTypes and CFFI allow the more complicated distributions to be used
directly in Numba after compiling the file distributions.c into a DLL or so.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to have been broken in some of the reorganization due to the includes changing.

@bashtage
Copy link
Contributor

A quick check

In [19]: import numpy as np
    ...: bg = np.random.PCG64()
    ...: bg.ctypes
    ...:
Out[19]: interface(state_address=140611115848248, state=c_void_p(140611115848248), next_uint64=<CFunctionType object at 0x7fe293dd6bb0>, next_uint32=<CFunctionType object at 0x7fe293dd6c80>, next_double=<CFunctionType object at 0x7fe293dd6d50>, bit_generator=c_void_p(140611115848176))

shows that the cffi and the ctypes interfaces to the bit generators are still there.

@rgommers rgommers added this to the 1.17.4 release. milestone Oct 31, 2019
@charris charris added the 08 - Backport Used to tag backport PRs label Nov 12, 2019
@rgommers @mattip
MAINT: don't install partial numpy.random C/Cython API.
afd577d 
See https://mail.python.org/pipermail/numpy-discussion/2019-September/080088.html
for discussion. We need to do this right, and add tests and docs.
All this PR does is not install bitgen.h, common.pxd and bit_generator.pxd
(they're still shipped in the sdist, that is needed).

Should be backported to 1.17.x

(cherry picked from commit 09ee875)
@rgommers @mattip
DOC: remove the docs about extending random with Cython/Numba.
495e2ee 
This was broken in 1.17.x and is being removed completely in 1.17.3
(to be reintroduced in 1.18.0, so this commit is not in master).
@rgommers @mattip
DOC: add note on numpy.random Cython API removal to 1.17.3 relnotes
281ee32
@mattip
Copy link
Member
mattip commented Nov 18, 2019

Rebased and touched up. The cffi and ctypes interfaces still are there so I left the reference. If one digs deeply they can expose the cython PyCapsule interface, but I do not think it is worth mentioning.

charris
charris reviewed Nov 18, 2019
View reviewed changes
numpy/random/src/distributions/distributions.h
@@ -9,7 +9,7 @@
#include "Python.h"
#include "numpy/npy_common.h"
#include "numpy/npy_math.h"
#include "numpy/random/bitgen.h"
#include "src/bitgen.h"
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct for 1.17, and it looks like 1.17 will be the same as 1.18 after the C-API changes go in.

charris@fc [numpy.git ((v1.17.4))]$ find . -name 'bitgen\.h'
./numpy/core/include/numpy/random/bitgen.h

Copy link
Member
@charris charris Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The current 1.17 file is renamed in this PR, do we want to do that for an existing release?

charris
charris reviewed Nov 18, 2019
View reviewed changes
numpy/random/common.pxd
@@ -5,7 +5,7 @@ from libc.stdint cimport (uint8_t, uint16_t, uint32_t, uint64_t,
uintptr_t)
from libc.math cimport sqrt

cdef extern from "numpy/random/bitgen.h":
cdef extern from "src/bitgen.h":
Copy link
Member

Choose a reason for hiding this comment

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

The original is what is in 1.17 and will also be in 1.18 after the C-API changes go in.

@charris
Copy link
Member
charris commented Nov 19, 2019

The location of the two include files differs from what is proposed for 1.18. This PR puts one in numpy/random/src and leaves the other in numpy/random/src/distributions. I suppose that is OK, but in 1.18 they will both be in numpy/core/include/numpy/random.

@rgommers
Copy link
Member Author

The location of the two include files differs from what is proposed for 1.18.

That's on purpose: we want the 1.17 one to be unreachable because it's not public API yet. It becomes public with cleaned up names in 1.18

@charris charris merged commit a068e7d into numpy:maintenance/1.17.x Nov 19, 2019
@charris
Copy link
Member
charris commented Nov 19, 2019

Thanks Ralf, Matti.

@rgommers rgommers deleted the bport-cythonapi branch November 19, 2019 02:50
@rgommers
Copy link
Member Author

Thanks a lot @mattip for finishing this up!

@charris charris removed this from the 1.17.5 release milestone Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charris charris charris left review comments

@mattip mattip mattip left review comments

@bashtage bashtage bashtage left review comments

Assignees
No one assigned
Labels
03 - Maintenance 08 - Backport Used to tag backport PRs component: numpy.random
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rgommers @bashtage @mattip @charris
0