8000 BUG: check for probabilities containing nan-value in random.choice by nritsche · Pull Request #11264 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: check for probabilities containing nan-value in random.choice #11264

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
Closed
wants to merge 1 commit into from
Closed

BUG: check for probabilities containing nan-value in random.choice #11264

wants to merge 1 commit into from

Conversation

nritsche
Copy link
@nritsche nritsche commented Jun 6, 2018

fixes #11250

eric-wieser
eric-wieser reviewed Jun 7, 2018
View reviewed changes
numpy/random/mtrand/mtrand.pyx Outdated
@@ -1144,6 +1144,8 @@ cdef class RandomState:
raise ValueError("probabilities are not non-negative")
if abs(kahan_sum(pix, d) - 1.) > atol:
raise ValueError("probabilities do not sum to 1")
if np.logical_or.reduce(np.isnan(p)):
raise ValueError("probabilities contain a value that is not a number")
Copy link
Member
@eric-wieser eric-wieser Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Easier fix - change the previous if to if not (abs(kahan_sum(pix, d) - 1) <= atol):

@nritsche nritsche force-pushed the check-for-nan-in-random-choice branch 2 times, most recently from 32938b5 to 84bea81 Compare June 7, 2018 17:12
@nritsche
Copy link
Author
nritsche commented Jun 7, 2018

thanks eric! i fixed that

@eric-wieser
Copy link
Member
eric-wieser commented Jun 7, 2018 •
edited
Loading

You should add a test too.

Can you add a comment explaining that negation is to handled nans?

@nritsche
Copy link
Author
nritsche commented Jun 7, 2018 •
edited by eric-wieser
Loading

I did, but I don't get the test to work. Do you understand what's wrong here?

>       assert_raises(ValueError, sample, [1, 2, 3], p=[None, None, None])

sample     = <built-in method choice of mtrand.RandomState object at 0x7fe48998b2d0>
self       = <numpy.random.tests.test_random.TestRandomDist object at 0x7fe497701090>

numpy/random/tests/test_random.py:406: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python2.7/unittest/case.py:473: in assertRaises
    callableObj(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   RuntimeWarning: invalid value encountered in less

Lock       = <built-in function allocate_lock>
RandomState = <type 'mtrand.RandomState'>
__builtins__ = <builtins>
__doc__    = None
__file__   = '/home/rik/numpy/build/testenv/lib/python2.7/site-packages/numpy/random/mtrand.so'
__name__   = 'numpy.random.mtrand'
__package__ = 'numpy.random'
__test__   = {'RandomState.binomial (line 3699)': '
        binomial(n, p, size=None)

        Draw samples from a binomial distrib....1, 0.3])
        array(['pooh', 'pooh', 'pooh', 'Christopher', 'piglet'],
              dtype='|S11')

        ", ...}

@seberg
Copy link
Member
seberg commented Jun 8, 2018

The comparison gives a warning in the tests, and we set up the tests to error on warnings (because we want to know about spurious warnings). Now, you probably do not care about the warning, so the easiest thing is to disable it from the numpy side using:

def test():
    setup_stuff
    with np.seterr(invalid='ignore'):
        function_call()

@nritsche nritsche force-pushed the check-for-nan-in-random-choice branch from 84bea81 to bbc0a03 Compare June 8, 2018 18:13
@nritsche nritsche force-pushed the check-for-nan-in-random-choice branch from bbc0a03 to 4c8642f Compare June 8, 2018 18:15
@nritsche
Copy link
Author
nritsche commented Jun 8, 2018

done, thanks!
I used with np.errstate(invalid='ignore'): though, is that correct?

eric-wieser
eric-wieser reviewed Jun 8, 2018
View reviewed changes
numpy/random/mtrand/mtrand.pyx
@@ -1142,7 +1142,9 @@ cdef class RandomState:
raise ValueError("a and p must have same size")
if np.logical_or.reduce(p < 0):
raise ValueError("probabilities are not non-negative")
if abs(kahan_sum(pix, d) - 1.) > atol:

# negated to handle NaNs in p
Copy link
Member
@eric-wieser eric-wieser Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking about it, this would make sense to do to the p < 0 above instead - that would make the warning dissappear too.

So if not np.all(p >= 0): raise ...

@mattip mattip mentioned this pull request Jun 21, 2018
Merged
@mattip
Copy link
Member
mattip commented Jul 3, 2018

@Googl1 this is waiting for a reaction to the suggested change

@mattip
Copy link
Member
mattip commented Aug 9, 2018

@nritsche ping

@nritsche nritsche closed this Aug 27, 2018
@jocelynbaduria jocelynbaduria mentioned this pull request Nov 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

Assignees
No one assigned
Labels
00 - Bug component: numpy.random
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Passing a list of None to np.random.choice(p=[None,None,None])
5 participants
@nritsche @eric-wieser @seberg @mattip @charris
0