I guess that " Fixes #16862 " is missing from description :)

To answer your question about why this is failing: the build log states:

sklearn/svm/_newrand.c:623:10: fatal error: newrand-cache.h: No such file or directory
 #include "newrand-cache.h"
          ^~~~~~~~~~~~~~~~~
compilation terminated.

Does the build work for you locally ? If so that means that you probably forgot to add this newrand-cache.h file ?

Note that you seem to also have forgotten the .cpp one.

Haha I had the tag in a comment so it didn't show. And yeah I was just an idiot. I was playing around with Cython and never changed it back before pushing.

Oh cool, didn't realize that's what that was.

Also, unrelated question. Is there a make target for running pull-request checks (pep8, tests, coverage, etc.)?

For testing you would need to build from source. You can find out more information in the contributing guide

Seems like the numpy version in the azure pipeline is 1.13.3 which doesn't have quantile. Does the pipeline use the same versions as the latest version of sklearn?

We test with multiple versions of numpy. For now our minimum supported version of numpy is 1.13. An alternative to quantile would be np.percentile.

Ah ok, thanks for the clarification!

Hey @smarie, I wanted to check in on this PR. What's left that needs done for this?

Hi @Justin-Huber , thanks for asking. It seems that my previous review comments were somehow hidden to you by github so I re-created both (one on the include header comment and one on the test per se). The comment concerning the test seems important: I discovered that p-values should be uniformly distributed under null hypothesis, so we can do a statistical test on the p-values uniformity itself. This does not add much but should provide a more understandable answer to the question "why a quantile" ?

If you're ok with these two proposals and merge them in, the rest is ok for me and I'll hand over to the "official" scikit learn reviewers (@thomasjpfan seems to be the one) for style and other nitpicks, since I do not master completely all official style rules in sklearn ;)

@Justin-Huber I think that your idea of adding a test for determinism is indeed great. Could you please complete it ? Other than that and a few nitpicks, LGTM :)

So I added the deterministic check back in which should only pass for the 32 bit mtrand19937 but it all passes so I think the mt19937_64 is never being created among all the tests. Are there any tests ran on a 64-bit system where the INT_MAX in random.h would be 64 bit?

(EDITED 24/06/2020 13:41)

Thanks @Justin-Huber for the update. I suggested a few minor changes concerning test names and docstring, see above.

Concerning your last question can you explain why you think your test_set_seed test should not pass for a mt19937_64 ? Is this because of the _MAX_UNSIGNED_INT seed ?

It seems from this reference that LONG_MAX would be more appropriate than INT_MAX to detect 64 bits systems in our _newrand.h file. However, reading back the results from our main source for newrand, and in particular the sentence just before the "conclusions" chapter:

[...] perhaps more importantly, these results also show that if you don't actually need 64-bit output, using a 64-bit PRNG is usually slower than using a 32-bit PRNG

So since current behaviour seems to be actually using always a 32 bits PRNG (which is recommended), maybe we should keep it like this and remove the currently unused mt19937_64 switch entirely. We would also need to add a comment in bounded_rand_int like this: // Note: orig_range is actually a 32 bits unsigned int so in [0; 4,294,967,295]

To conclude, according to what you have found, I suggest to do the following things:

• Remove the mt19937_64 switch as explained above
• Change the bounded_rand_int function in _newrand.h to at least include a comment as suggested above, or to change its signature as inline uint32_t bounded_rand_int(uint32_t orig_range) {
• Change the _newrand.pyx so that unsigned or unsigned int (more readable) is used everywhere in this wrapper instead of int
• Add the case (_MAX_UNSIGNED_INT, 10000) to test_bounded_rand_int. It should now work (before the above mod it would lead to an overflow)
• Add two negative tests showing which values can overflow the wrapper:

@pytest.mark.parametrize('seed',
                         [-1, _MAX_UNSIGNED_INT + 1])
def test_set_seed_overflow(seed):
    """Test that `set_seed_wrap` is defined for unsigned 32bits ints"""
    with pytest.raises(OverflowError):
        set_seed_wrap(seed)

@pytest.mark.parametrize('orig_range',
                         [-1, _MAX_UNSIGNED_INT + 1])
def test_bounded_rand_int_limits(orig_range):
    """Test that `bounded_rand_int_wrap` is defined for unsigned 32bits ints"""
    with pytest.raises(OverflowError):
        bounded_rand_int_wrap(orig_range)

What do you think ?

I would love to get the feedback from people more at ease with C/C++ than myself here, and involved in the original PR. Maybe @jnothman and @thomasjpfan ?

Note for reference: in order to double-check that current implementation does not accept more than a 32 bits unsigned int for orig_range, I temporarily unlocked the types to unsigned long long in the Cython wrapper and ran the uniformity test with _MAX_UNSIGNED_INT * 2. It correctly failed : all numbers returned were below _MAX_UNSIGNED_INT. Such a double-check may seem naive but at least I'm now confident in the above proposals :)

Added the changes, let me know what you think. :)

Great @Justin-Huber ! Apart from two remaining comments (see above), looks very good to me. Almost there! Thanks for your patience

Awesome I made those last style fixes :)

You're welcome @adrinjalali :)
Thanks @Justin-Huber !

(note for what's worth: two style comments from @jnothman seem to have been forgotten - maybe this can be done by a maintainer directly on master ?)

Awesome! It's been a pleasure, thanks for the help! @smarie :)