8000 [stdlib] Introduce `random.shuffle` for `List` by jjvraw · Pull Request #3327 · modular/modular · GitHub
[go: up one dir, main page]

Skip to content

[stdlib] Introduce random.shuffle for List #3327

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.

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

Closed
wants to merge 3 commits into from

Conversation

jjvraw
Copy link
Contributor
@jjvraw jjvraw commented Jul 29, 2024

Introduce random.shuffle for List. Implementation follows the Fisher-Yates shuffle.

@jjvraw jjvraw requested a review from a team as a code owner July 29, 2024 11:50
@jjvraw
Copy link
Contributor Author
jjvraw commented Jul 29, 2024

For concerns regarding the Fish 8000 er-Yates algorithm, one can refer to a corresponding issue in the CPython repository. In summary, the Python community appears to be satisfied with this implementation.

@jjvraw jjvraw force-pushed the random-shuffle branch 2 times, most recently from 7789dee to 87a6757 Compare July 29, 2024 12:52
@soraros
8000 Copy link
Contributor
soraros commented Jul 29, 2024

Can we set a seed to make the tests deterministic?

@jjvraw
Copy link
Contributor Author
jjvraw commented Jul 29, 2024

Are you suggesting the same seed for all tests? Is the concern about potential failing assertions due to randomness?

It might be beneficial to introduce some seeded tests, or perhaps modify some existing ones to seeded. Currently we rely on an extremely low chance of a shuffled list resulting in the same order to assert success. There is more confidence that a failed test case indicates an ineffective shuffle. Alternatively, using property tests, followed by sorting and checking for equality with the original list, might be more reliable. What do you think?

@soraros
Copy link
Contributor
soraros commented Jul 29, 2024

I don't have a strong opinion about this, and if we do move to explicit seeding, we don't have to do it in this PR. I'm just taking the chance to bring it up.

My main point is: if the tests are deterministic, we can simplify them into something like this:

seed()
var l = List(1, 2, 3, 4)
shuffle(l)
assert_equal(l, List(2, 3, 1, 4))  # we know beforehand the result we will get

Signed-off-by: Joshua James Venter <venter.joshua@gmail.com>
Signed-off-by: Joshua James Venter <venter.joshua@gmail.com>
@JoeLoser JoeLoser self-assigned this Nov 16, 2024
Signed-off-by: Joe Loser <joe@modular.com>
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Nov 22, 2024
@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels Nov 22, 2024
modularbot pushed a commit that referenced this pull request Nov 23, 2024
[External] [stdlib] Introduce `random.shuffle` for `List`

Introduce `random.shuffle` for `List`. Implementation follows the
Fisher-Yates shuffle.

Co-authored-by: Joshua James Venter <venter.joshua@gmail.com>
Closes #3327
MODULAR_ORIG_COMMIT_REV_ID: 911eadee8d34911f653e8c69f85fcd89f5cae344
@modularbot
Copy link
Collaborator

Landed in ba0f514! Thank you for your contribution 🎉

@modularbot modularbot closed this Nov 23, 2024
modularbot pushed a commit that referenced this pull request Dec 17, 2024
[External] [stdlib] Introduce `random.shuffle` for `List`

Introduce `random.shuffle` for `List`. Implementation follows the
Fisher-Yates shuffle.

Co-authored-by: Joshua James Venter <venter.joshua@gmail.com>
Closes #3327
MODULAR_ORIG_COMMIT_REV_ID: 911eadee8d34911f653e8c69f85fcd89f5cae344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0