Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Jul 17, 2020

This ports the sampling algorithm to ranges.

I am unsure what exactly the problem is with the tests. I guess somecombinations of the machinery do not play well with the requirements but I have to figure out which.

@miscco miscco requested a review from a team as a code owner July 17, 2020 12:42
@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Jul 17, 2020
@CaseyCarter CaseyCarter mentioned this pull request Jul 17, 2020
@StephanTLavavej
Copy link
Member

Thanks for the initial review, @statementreply! 😸 Moving to WIP as you've identified the cause of (at least one of) the test failures (mentioning a nonexistent _It).

@miscco
Copy link
Contributor Author

miscco commented Jul 19, 2020

It seems activating the tests actually helps...

miscco and others added 2 commits July 19, 2020 13:43
Co-authored-by: statementreply <statementreply@gmail.com>
@miscco miscco changed the title Implement ranges::sample Implement ranges::sample and ranges::sample Jul 19, 2020
@miscco miscco changed the title Implement ranges::sample and ranges::sample Implement ranges::sample and ranges::shuffle Jul 19, 2020
@miscco
Copy link
Contributor Author

miscco commented Jul 19, 2020

Given that we move the concept definition into algorithm I have put the ranges::shuffle implementation into this PR as otherwise merging is a hassle and both are similar anyway

@miscco
Copy link
Contributor Author

miscco commented Jul 19, 2020

Tests are active and passing so this should be ready for review

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Just a nitpick which I'll go ahead and apply.

Avoid contextually converting integer to bool.
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good modulo comments, thanks!

@StephanTLavavej StephanTLavavej removed their assignment Jul 23, 2020
@CaseyCarter CaseyCarter self-assigned this Jul 27, 2020
@CaseyCarter CaseyCarter merged commit 99241dc into microsoft:master Jul 27, 2020
@CaseyCarter
Copy link
Contributor

Thanks for implementing these algorithms, whose choice I suspect was actually quite deliberate - not random at all.

@CaseyCarter CaseyCarter removed their assignment Jul 27, 2020
@miscco miscco deleted the ranges_sample branch July 27, 2020 18:48
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Jul 28, 2020
Co-authored-by: statementreply <statementreply@gmail.com>
@eugnsp
Copy link

eugnsp commented Feb 27, 2025

The new loop termination condition in Sample_selection_unchecked is wrong. The loop should be terminated when Count hits zero, not PopSize. The current implementation leads to redundant RngFunc(PopSize) calls, just to check the condition RngFunc(PopSize) < 0, which will never be satisfied. This results in redundant calls to a random number generator after the sampling is already done and to unreasonable deviations in behaviour between 16.7 and 16.8. The average number of such redundant calls is about [InputRangeSize] / ([OutputRangeSize] + 1), which could be significant if a small number of elements is requested.

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

Labels

cxx20 C++20 feature ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants