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

Enable to set random ranges to RandomEmojiOverlay parameters #177

Closed
wants to merge 46 commits into from

Conversation

lyakaap
Copy link
Contributor

@lyakaap lyakaap commented Dec 16, 2021

Summary

  • I have read CONTRIBUTING.md to understand how to contribute to this repository :)

Added randomness to RandomEmojiOverlay parameters like emoji_size.
A motivation is that I wanted to randomly change the parameters on-the-fly, rather than "fixed" like current implementation.
This modified augmentation was actually used in ISC21 Descriptor Track 1st-place solution.

Test Results

The tests are passed except the following error.

Traceback (most recent call last):
  File "/Users/shuhei.yokoo/Documents/AugLy/augly/tests/image_tests/transforms_unit_test.py", line 177, in test_RandomEmojiOverlay
    self.evaluate_class(
  File "/Users/shuhei.yokoo/Documents/AugLy/augly/tests/image_tests/base_unit_test.py", line 129, in evaluate_class
    are_equal_images(dst, ref), "Expected and outputted images do not match"
  File "/Users/shuhei.yokoo/Documents/AugLy/augly/tests/image_tests/base_unit_test.py", line 20, in are_equal_images
    return a.size == b.size and np.allclose(np.array(a), np.array(b))
  File "<__array_function__ internals>", line 5, in allclose
  File "/Users/shuhei.yokoo/.pyenv/versions/augly/lib/python3.8/site-packages/numpy/core/numeric.py", line 2249, in allclose
    res = all(isclose(a, b, rtol=rtol, atol=atol, equal_nan=equal_nan))
  File "<__array_function__ internals>", line 5, in isclose
  File "/Users/shuhei.yokoo/.pyenv/versions/augly/lib/python3.8/site-packages/numpy/core/numeric.py", line 2358, in isclose
    return within_tol(x, y, atol, rtol)
  File "/Users/shuhei.yokoo/.pyenv/versions/augly/lib/python3.8/site-packages/numpy/core/numeric.py", line 2339, in within_tol
    return less_equal(abs(x-y), atol + rtol * abs(y))
ValueError: operands could not be broadcast together with shapes (1080,1920,3) (1080,1920,4) 

----------------------------------------------------------------------
Ran 71 tests in 35.609s

FAILED (errors=1, skipped=4)
sys:1: ResourceWarning: unclosed file <_io.BufferedReader name='/Users/shuhei.yokoo/Documents/AugLy/augly/assets/tests/image/inputs/dfdc_1.jpg'>

It seems that expected image has alpha channel whereas output image doesn't have alpha channel.
I'm not sure why it happened. Replacing the expected image is needed?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2021
@lyakaap lyakaap changed the title [WIP] Enable to set random ranges to RandomEmojiOverlay parameters Enable to set random ranges to RandomEmojiOverlay parameters Dec 16, 2021
Copy link
Contributor

@zpapakipos zpapakipos left a comment

Choose a reason for hiding this comment

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

Hi @lyakaap, thanks so much for this PR! :)

I left a couple comments with small changes I'd like before landing this. You will indeed need to replace the expected output file for the RandomEmojiOverlay unit test. You can do this e.g. by temporarily adding a line pil_dst.save("augly/assets/tests/image/dfdc_expected_output/test_RandomEmojiOverlay.png") here and then running python -m unittest augly.tests.image_tests.functional_unit_test.FunctionalImageUnitTest.test_RandomEmojiOverlay. Then if that passes, remove the line you added in base_unit_test.py and commit your changes (should just be the new test_RandomEmojiOverlay.png file)!

augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/transforms.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
@lyakaap
Copy link
Contributor Author

lyakaap commented Dec 18, 2021

Thank you for your quick & kind review!

I reflected your review and confirmed the tests are passed.
Like you instructed, I rewrote png file by inserting the line, but in evaluate_class instead evaluate_function.
And executed python -m unittest augly.tests.image_tests.transforms_unit_test.TransformsImageUnitTest.test_RandomEmojiOverlay.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@zpapakipos zpapakipos left a comment

Choose a reason for hiding this comment

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

Sorry for the delay with reviewing this PR, I was on PTO for the holidays! :) Please fix the nits I suggested below, and also run black on the files which are failing in the lint workflow, as you can see.

black augly/image/functional.py
black augly/tests/image_tests/transforms_unit_test.py

augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
augly/image/functional.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@lyakaap has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@zpapakipos has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants