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.

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

Moved Shuffle to python 3.11 #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Vishalk91-4
Copy link

@bennn

Fixed #11

I have changed shuffle to one argument, could you please review it

Copy link
Member

@bennn bennn left a comment

Choose a reason for hiding this comment

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

These changes ignore the order argument.

@Vishalk91-4
Copy link
Author

These changes ignore the order argument.

@bennn, since in Python 3.11 the order function isn't required for shuffle
So, I removed it

@bennn
Copy link
Member

bennn commented Jan 31, 2024

yes, but removing order altogether changes the behavior of the code

can you write a test to show the behavior is the same before & after?

Copy link
Contributor

@mrigankpawagi mrigankpawagi left a comment

Choose a reason for hiding this comment

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

This is not equivalent when order is non-zero (and so s == order), because the order-argument determines the way shuffling will take place. However from the point of view of the benchmark, I do not think it matters... The script passes 0.5 and has deterministic shuffles. Without the order-argument, the shuffling is still determinstic (due to the seed) except that it is a different (again, deterministic) permutation.

For reference, the original implementation is here and is quite simple. You can choose to create a new shuffle function (by essentially taking the code from the link before) which takes the order argument.

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.

shuffle vs. python 3.11
3 participants