[xrandom] Improve random::choice
#1011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks! FYI It seems that your commit email does not correspond top the your GitHub account email. If we merge this as it is, you won't be credited for the commit. |
wolfv
reviewed
Jul 30, 2018
| for(auto it = de.storage().begin() + n; it != de.storage().end(); ++it, ++i) | ||
| { | ||
| auto idx = std::uniform_int_distribution<std::size_t>(0, i)(engine); | ||
| if (idx < n) |
There was a problem hiding this comment.
we require brackets even around one liners! sry :)
|
Looks great! just the single nitpick... |
|
I'll merge after the tests show up green! |
|
Congrats! 🎉 |
|
Thanks @wolfv! :) |
FrozenWinters
pushed a commit
to FrozenWinters/xtensor
that referenced
this pull request
Aug 14, 2018
Added resevoir sampling and sampling with replacement with corresponding tests
FrozenWinters
pushed a commit
to FrozenWinters/xtensor
that referenced
this pull request
Aug 14, 2018
Avoided installation failure in absence of nlohmann_json Fix more warnings (xtensor-stack#999) * refactor strides computation * use better temporary type for strided view * remove rebind container for fixed shape type Shared xexpression + automatic xclosure detection (xtensor-stack#992) add xshared expression Update view.rst convertible types instead of same type basic usage replaced with getting started fix where on xview [xrandom] Improve `random::choice` (xtensor-stack#1011) Added resevoir sampling and sampling with replacement with corresponding tests Add tests to evaluate shared expression [xnpy] Add `signed char` to deserialization format (xtensor-stack#1017) * Add int8 support to xnpy [xexpression] - Fix make_xshared (xtensor-stack#1024) Fix make_xshared [docs] Kwargs don't exist in C++ Quick nit about the documentation on numpy -> xtensor xshared_expression fix (xtensor-stack#1025) xshared_expression fixed changelog updated Release 0.17.0 Update binder and releasing instructions missing comparison operators for xlices fix rebind_container for gcc8 c++17 .. functor return value fixed fix up shape_type in case xtensor fixed is used fix up shape_type in case xtensor fixed is used, use std::array for xtensor_fixed in MSVC add more adapt functions add docs + tests fix strided view on view fix warnings for braces on older clang versions Improved CRTP base class detection add std qualifiers to size_t and ptrdiff_t ifdef copysign for MSVC release 0.17.1 [docs] tanh typo Quick nit add missing parenthesis around std::min and std::max
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR corresponds to the following fixes:
The garbage values produced by this issue is due to an optional assert. We throw
std::runtime_errorinstead.Previously, we performed a Fisher Yates shuffle on the array and then copied the shuffle. As @wolfv has pointed out, the stdlib implements it using reservoir sampling, which we implement here.
We also add an option to perform sample with replacement, and make it true by default, since that is numpy's behavior