Repository navigation
randomkit.h is not included in NumPy 1.17 builds #14517
Description
There has been a recent StackOverflow question what happened to the randomkit.h that was included in the numpy/random directory of the binary distributions of NumPy < 1.17 and isn't included in NumPy 1.17+.
The removal of the header file wasn't mentioned in the changelog so I wonder if that was accidental or intentional and if it should have been mentioned in the changes.
Activity
charris commentedon Sep 15, 2019
Hmm, I suspect it got lost. Looks like in 1.16.x it was added in
numpy/random/setup.py.And that is missing in 1.17.x.
charris commentedon Sep 15, 2019
@MSeifert04 That said, I don't think the random internals were meant to be used downstream, which seems to be the case with the StackOverflow user. Perhaps the best solution to his problem would be to copy the relevant files.
mattip commentedon Sep 15, 2019
The recommended use of cython, numba and cffi with numpy.random is now documented, and we even have some examples. The examples should be somehow linked to the documentation, suggestions welcome.
paul-the-noob commentedon Sep 15, 2019
(I am that SO user.)
numpy.random.distributions.pxdwhich one of the examples cimports seems to be missing from 17.2 binary dist.common.pxdandbit_generator.pxdare there,legacy_distributions.pxdandbounded_integers.pxdare not. The last one looks relevant to my use case.mattip commentedon Sep 15, 2019
Thanks for the feedback. That seems like an oversight.
mattip commentedon Sep 15, 2019
@bashtage should we be including more
pxdfiles?bashtage commentedon Sep 15, 2019
I do not believe that randomkit is used any more. C and h should be removed from 1.17.
bashtage commentedon Sep 15, 2019
The question about including the pxd comes does to whether the low level api side of things is to be supported. These simplify using the numpy random infrastructure in cython.
paul-the-noob commentedon Sep 15, 2019
Should you decide not to officially support these interfaces, you probably want to mention this in the docs/examples referenced above.
rgommers commentedon Sep 17, 2019
It would be nice to revisit the API here even if we do keep it, there's things like
The
commonlooks weird;distributionsmakes sense as a name but doesn't mirror the Python API, and I'm not sure what therandom_gauss_zigname is about.charris commentedon Sep 17, 2019
Probably
random_normal_zigguratwould be better :) I'm in favor of having an API if we did before, apparently it was used. But the naming and organization should be laid out.bashtage commentedon Sep 18, 2019
Certainly could use a clean-up.
random_gauss_zigshould berandom_gauss. It wasn'trandom_gausssince this name was taking by the BM normal generator at the time. If the legagy generator is rolled back to the 1.16 version, then naming may get a little simpler.bitgen_tshould probably have its ownpxdfile, but was addedcommonto avoid creating another pxd file.commonis a set of functions shared by Generator-like. Should be sort of private, but it useful is someone wants to write a custom Generator since it exposes methods to automagically handle all broadcasting.I think
randomkit, if restored, should probably remain undocumented at the API level.rgommers commentedon Sep 18, 2019
Then let's not restore it ....
This seems to be the algorithm that implements
Generator.standard_normal, so why notstandard_normal?In general, the Python and Cython API should match unless there's a good reason to deviate. And imho that should apply to function/method names and namespaces alike.
bashtage commentedon Sep 18, 2019
I'm not 100% on how name resolution works in Cython. Probably OK , at least if we use a module,
distributions.standard_normalinstead of juststandard_normal. Therandom_prefix is inherited fromrandomkit. This is probably the right idea and solves most of the bikeshedding.Having though a bit harder I don't think it is possible to go down that path. See #14531 (comment)
6 remaining items
rgommers commentedon Oct 10, 2019
Vendored code vs self-written code is irrelevant. What matters is: does it look like a public API or not. If not, change it or rip it out any time. If it does (or is ambiguous), it needs to be treated as such.
In this case, the only headers that are public are those you'll find through
np.get_include(). I think this is documented well enough.randomkit.hwas innumpy/random/not innumpy/core/include/numpy/(which is whatnp.get_include()points to). Sorandomkit.hwas private (admittedly, would have been better not to install it at all though). If it was easy to restore, we probably would (if we get one bug report that someone was relying on it, there may be more users relying on it), but it seems that it's not.paul-the-noob commentedon Oct 13, 2019
@mattip
I just assumed from the name that it is the "backend" to
randint. If that's not the case orrandintlike functionality is available elsewhere that's fine with me.mattip commentedon Oct 13, 2019
rand_intis now supplied by_bounded_integers, but you need to choose your exact int type. @paul-the-noob could you describe your use case in a little more detail so we can make sure to include it as part of gh-14604? Are you building a BitGenerator to reuse in NumPy's Generator, or sampling uniform integers of a specific C type to be used directly by your code, or creating your own distributions?bashtage commentedon Oct 13, 2019
paul-the-noob commentedon Oct 13, 2019
@mattip
I'm essentially creating permutations or shuffling vectors subject to certain constraints. IIRC, one reason I decided to use Cython was that at the time
randintdidn't support array iimits. But there are other factors that make the code "too sequential for numpy".To summarize I need to draw random ints of a given C type from continually changing ranges, either one-by-one or small batch-by-small batch.
mattip commentedon Oct 27, 2019
The recommended C-API was documented in #14604, this is the documentation from that PR. I think you are looking for
int64_t random_positive_int64(bitgen_t *bitgen_state)and friends for a single value?charris commentedon Nov 12, 2019
@mattip What was the decision on this for the random C-API?
mattip commentedon Nov 12, 2019
I think we can close it, we have an alternative for users who needed this and #14778 widens the use cases for this and other ways to call into the numpy.random C-API
charris commentedon Nov 12, 2019
OK, will close. This does introduce an discontinuity from 1.16 to 1.18, correct? If so, it would be good to mention it somewhere in the release notes where it will be noticed, highlights maybe. Do we have a way to add highlights?
mattip commentedon Nov 12, 2019
According to the
doc/release/ucoming_changes/README.rst, we can add annn.highlight.rst