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.

Sign up for GitHub

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

Jump to bottom

Add RandomSampler to Samplers #952

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Add RandomSampler to Samplers #952

merged 5 commits into from
Apr 4, 2023

Conversation

abuelnasr0
Copy link
Contributor

for #946

  • random_sampler.py class
  • random_sampler_test.py class with passing unit tests

@mattdangerw
Copy link
Member

/gcbrun

mattdangerw
mattdangerw requested changes Apr 1, 2023
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Overall looks good! Couple comments

keras_nlp/samplers/random_sampler.py Outdated Show resolved Hide resolved
keras_nlp/samplers/random_sampler.py Outdated Show resolved Hide resolved
@mattdangerw mattdangerw self-assigned this Apr 1, 2023
chenmoneygithub
chenmoneygithub suggested changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

At a high level, we prefer splitting commits doing different things into different PRs, which would be cleaner to track.

keras_nlp/samplers/__init__.py Outdated Show resolved Hide resolved
keras_nlp/samplers/beam_sampler.py Outdated Show resolved Hide resolved
chenmoneygithub
chenmoneygithub approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, 2 minor comments.

keras_nlp/samplers/random_sampler.py Outdated Show resolved Hide resolved
keras_nlp/samplers/random_sampler.py Outdated Show resolved Hide resolved
@abuelnasr0 abuelnasr0 requested a review from mattdangerw April 3, 2023 21:54
@mattdangerw
Copy link
Member

Very nice! Thank you.

@mattdangerw mattdangerw merged commit 9dc8b58 into keras-team:master Apr 4, 2023
@abuelnasr0 abuelnasr0 deleted the Add-RandomSampler branch December 28, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenmoneygithub chenmoneygithub chenmoneygithub approved these changes

@mattdangerw mattdangerw Awaiting requested review from mattdangerw

Assignees

@mattdangerw mattdangerw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abuelnasr0 @mattdangerw @chenmoneygithub