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 probability param for random flip functions. #38

Merged
merged 9 commits into from May 20, 2022
Merged

Add probability param for random flip functions. #38

merged 9 commits into from May 20, 2022

Conversation

pabloduque0
Copy link
Collaborator

@pabloduque0 pabloduque0 commented May 9, 2022 •
edited

Based on the conversation in #37 . Will work on adding one or two functions in the upcoming weeks.

Apologies for the extra unnecessary commits, vs studio butcher the notebook and had to revert that one, we can squash it. Happy to update the notebook in a follow up as well.

A few thoughts:

  • I have not added checks (asserts) on the prob value to allow jitting without needing static args. Let me know if this is okay, Im not super familiar with chex so maybe Im missing something there.
  • I had to change the way the extra parameters are evaluated in the test to kwargs always. I think this should be fine as long as we dont make some of those parameter positional only. Let me know if you rather take a different approach on this one.
  • I love how clever the test setup is! 🚀🚀🚀

Feel free to add any comments on the wording of the docstring as well.

@claudiofantacci claudiofantacci self-assigned this May 9, 2022
@claudiofantacci claudiofantacci self-requested a review May 9, 2022 21:42
claudiofantacci
claudiofantacci reviewed May 10, 2022
View reviewed changes
Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Thanks, great PR! 🚀

Can you please either:

  • remove the commits where you first and and then remove code
  • squash the commits into a single one

dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Show resolved Hide resolved
@pabloduque0
Add kw only probability param to random_flip_left_right and random_…
d1b13c8 
…flip_up_down.

Add example of usage of probability in flip functions in notebook.
Minor fix in docstring of `random_flip_left_right`.
Revert "Add example of usage of probability in flip functions in notebook."

This reverts commit eefb6ee.

Remove unnecessary import.
Changes based on PR comments.
.
@pabloduque0
Merge remote-tracking branch 'origin/master'
22cb6fc 
.
@pabloduque0
Copy link
Collaborator Author

pabloduque0 commented May 10, 2022 •
edited

Thanks a lot for the review! Let me know if I missed anything :)

claudiofantacci
claudiofantacci requested changes May 10, 2022
View reviewed changes
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
@pabloduque0
Copy link
Collaborator Author

Let me know if there is anything left to iron out :)

claudiofantacci
claudiofantacci requested changes May 10, 2022
View reviewed changes
Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Final comments! 🚀

dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
dm_pix/_src/augment.py Outdated Show resolved Hide resolved
claudiofantacci
claudiofantacci approved these changes May 10, 2022
View reviewed changes
Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Thanks a lot @pabloduque0! 🚀

@claudiofantacci
Copy link
Collaborator

Once internal review is passed, the PR will be automatically merged!
It might take a bit of time, but rest assure it will go through 😄
Thanks again for contributing, and feel free to open any other PR! 🚀

@claudiofantacci
Copy link
Collaborator

@pabloduque0, we have a release freeze in effect for NeurIPS 2022 until end of 2022/05/19.

After that date, the PR will be automatically merged!
Thanks for you patience 😄

@pabloduque0
Copy link
Collaborator Author

@claudiofantacci no worries at all! No rush from my side, after all I have the changes locally to use them :)

Yes, I will look into adding a couple functions in the near future, might follow up on #37 to check with you if they are a good fit or not.

Thanks a lot again for the review and the merge!

@copybara-service copybara-service bot merged commit 54b072b into google-deepmind:master May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudiofantacci claudiofantacci claudiofantacci approved these changes

Assignees

@claudiofantacci claudiofantacci

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pabloduque0 @claudiofantacci