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.

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

ENH: Add broadcast support to Generator.multinomial #16740

Merged
merged 3 commits into from
Nov 13, 2021
Merged

ENH: Add broadcast support to Generator.multinomial #16740

merged 3 commits into from
Nov 13, 2021

Conversation

bashtage
Copy link
Contributor

@bashtage bashtage commented Jul 3, 2020

Add broadcasting support to multinomial

xref #15201

@bashtage bashtage force-pushed the broadcast-multinomial branch 5 times, most recently from 9986d3e to 15e3965 Compare July 3, 2020 11:48
@yoavram
Copy link

yoavram commented Jul 3, 2020

I’d be happy to be a test user when this is ready

@bashtage bashtage force-pushed the broadcast-multinomial branch 2 times, most recently from dba9ebb to 5bfa6e1 Compare July 4, 2020 22:01
@mattip
Copy link
Member

mattip commented Aug 2, 2020

LGTM, but there is a merge conflict.

@bashtage
Copy link
Contributor Author

bashtage commented Aug 2, 2020

@mattip Is this fail due to setuptools/distutils expected?

@charris
Copy link
Member

charris commented Aug 3, 2020

Is this fail due to setuptools/distutils expected?

Should be fixed in master, we don't allow the troublesome version of setuptools. I'll try a close/reopen, if that doesn't work you will probably need to rebase.

@charris charris closed this Aug 3, 2020
@charris charris reopened this Aug 3, 2020
@bashtage
Copy link
Contributor Author

bashtage commented Aug 3, 2020

Funny thing is that this was just rebased on master when the fails started.

@mattip
Copy link
Member

mattip commented Aug 3, 2020

It seems setuptools 49.2.1 is out but issue pypa/setuptools#2259 is not fixed. We will have to pin setuptools<49.2.0.

@mattip
Copy link
Member

mattip commented Aug 3, 2020

xref pr gh-16993

@WarrenWeckesser WarrenWeckesser mentioned this pull request Oct 29, 2020
Open
WarrenWeckesser
WarrenWeckesser reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

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

I made a few suggestions, but I haven't carefully reviewed the implementation of the broadcasting. I did run into problems with a couple edge cases: rng.multinomial([1, 2], []) raises ZeroDivisionError: division by zero, which is a strange error to get for that input, and more seriously, rng.multinomial(5, []) crashes Python with Floating point exception: 8 on OSX and Floating point exception (core dumped) on Linux.

numpy/random/_generator.pyx Outdated Show resolved Hide resolved
numpy/random/_generator.pyx Outdated Show resolved Hide resolved
numpy/random/_generator.pyx Outdated Show resolved Hide resolved
numpy/random/_generator.pyx Outdated Show resolved Hide resolved
@bashtage
Copy link
Contributor Author

I think I have addressed all of the issues you identified @WarrenWeckesser . I also added checks for edge cases with empty arrays.

@bashtage bashtage force-pushed the broadcast-multinomial branch 2 times, most recently from ce55453 to 10ecb05 Compare February 18, 2021 16:38
Base automatically changed from master to main March 4, 2021 02:04
@bashtage bashtage force-pushed the broadcast-multinomial branch 2 times, most recently from 8a7e21b to e8ad2b1 Compare November 4, 2021 10:46
WarrenWeckesser
WarrenWeckesser requested changes Nov 8, 2021
View reviewed changes
Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

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

@bashtage, sorry this "dropped off my radar" (as they euphemistically say) for so long.
I made a few small suggestions, otherwise looks good.

numpy/random/_generator.pyx Outdated Show resolved Hide resolved
numpy/random/_generator.pyx Outdated Show resolved Hide resolved
numpy/random/tests/test_generator_mt19937.py Outdated Show resolved Hide resolved
numpy/random/tests/test_generator_mt19937.py Outdated Show resolved Hide resolved
numpy/random/tests/test_generator_mt19937.py Outdated Show resolved Hide resolved
@bashtage bashtage force-pushed the broadcast-multinomial branch 3 times, most recently from a5bd460 to 9eec91f Compare November 11, 2021 18:44
Kevin Sheppard and others added 3 commits November 12, 2021 09:35
@bashtage
ENH: Broadcast multinomial
c1b8f4b 
Add broadcasting support to multinomial

xref numpy#15201
@bashtage
ENH: Improve broadcasting in multinomial
25dfbc7 
Improve robustness fo broadcasting
Explicitly rule out impossible cases
@bashtage
ENH: Use broadcast_shape
43110b6 
Switch to broadcast_shape plus other small changes
@bashtage bashtage closed this Nov 12, 2021
@bashtage bashtage reopened this Nov 12, 2021
WarrenWeckesser
WarrenWeckesser approved these changes Nov 13, 2021
View reviewed changes
Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

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

Thanks @bashtage!

@WarrenWeckesser WarrenWeckesser merged commit aa52dee into numpy:main Nov 13, 2021
@bashtage
Copy link
Contributor Author

Thanks @WarrenWeckesser

@bashtage bashtage deleted the broadcast-multinomial branch November 13, 2021 08:42
howjmay pushed a commit to howjmay/numpy that referenced this pull request Nov 14, 2021
@bashtage @howjmay
ENH: random: Add broadcast support to Generator.multinomial (numpy#16740
44fb94e 
)

xref github issue numpy#15201
@jcusick13 jcusick13 mentioned this pull request Jun 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WarrenWeckesser WarrenWeckesser WarrenWeckesser approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@bashtage @yoavram @mattip @charris @WarrenWeckesser