Skip to content

This is PR #2727, but rebased#2793

Merged
certik merged 5 commits intonumpy:masterfrom
certik:pull-request-2727
Dec 13, 2012
Merged

This is PR #2727, but rebased#2793
certik merged 5 commits intonumpy:masterfrom
certik:pull-request-2727

Conversation

@certik
Copy link
Contributor

@certik certik commented Dec 6, 2012

Also I regenerated the cython file, just in case. We need to run tests on this one to make sure that all passes.

The size argument to random.choice should work like it does for all
other functions in random as well.
Random choice used np.unique to find new indices when replace
was False and p given. This is wrong since unique will sort the
indices. This solves the bug, but likely not ideal.
Thanks to @alan-isaac for pointing out the 0-d vs. scalar issue.
@certik
Copy link
Contributor Author

certik commented Dec 13, 2012

Hm, Travis didn't run tests.... I pushed in a "Travis ping" commit. Here we go.

@certik
Copy link
Contributor Author

certik commented Dec 13, 2012

Ok, all tests passed, so I am removing the "ping" commit and merging.

certik added a commit that referenced this pull request Dec 13, 2012
@certik certik merged commit cdeb48b into numpy:master Dec 13, 2012
@seberg
Copy link
Member

seberg commented Dec 13, 2012

@certik it was intentional to push this to master and not 1.7.? I just wanted to double check that the results look reasonable and wondered about them being nonsense before realizing that this went into master.

@certik
Copy link
Contributor Author

certik commented Dec 14, 2012

All bug fixes go into master first. I just rebased your original PR, which was also against master.

Should this be backported to 1.7 as well?

@seberg
Copy link
Member

seberg commented Dec 14, 2012

Ah, thanks... I would think it should be in 1.7., because it changes
what is returned by it and fixes a bug and the function is new in 1.7.

@certik certik mentioned this pull request Dec 14, 2012
@certik
Copy link
Contributor Author

certik commented Dec 14, 2012

Ok. Backported in #2820.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants