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

[py-tx] Create rotation helper functions in PhotoContent using Pillow #1671

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

haianhng31
Copy link
Contributor

@haianhng31 haianhng31 commented Oct 28, 2024

Summary

This is a part of the issue #1665
This has been discussed previously in PR #1669

  • Create RotationType enum in content_base.py
    • Use StrEnum to make the enum’s values independent of the order
  • Create 6 helper functions in PhotoContent
    • 5 functions are to rotate the image in different angles
    • 1 function is to try all different rotations

Test plan

@Dcallies said not to write a unittest at this stage.

However, I was able to show that the code is correct by writing a simple bit of python:

from threatexchange.content_type.photo import PhotoContent

file_path = pathlib.Path("threatexchange/tests/hashing/resources/LA.png")
with open(file_path, "rb") as f:
    image_data = f.read()

all_rotations = PhotoContent.all_simple_rotations(image_data)

for rotation_type, image_data in all_rotations.items():
    with open(f"image_{rotation_type}.png", "wb") as w:
        w.write(image_data)

Then I visually inspected all the files to confirm they were rotated as expected.

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

Looking good!

To test this, on your development environment, create a simple image, and then call, via Ipython the method and write the resultant bytes to files.

You may find that for some image types, you need to hint pillow on what the format is, which can be done via filename.

You can write the bytes to files and then visually inspect that you have all the rotations! You can include that code in your test plan.

@classmethod
def rotate_image(cls, image_data: bytes, angle: float) -> bytes:
"""
Rotate an image by a given angle
Copy link
Contributor

Choose a reason for hiding this comment

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

Include a note on why we might want to do this :P

Namely, this is help for testing hash algorithms.

return buffer.getvalue()

@classmethod
def try_all_rotations(cls, image_data: bytes):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Let's go with all_simple_rotations

@classmethod
def try_all_rotations(cls, image_data: bytes):
"""
Try all possible rotations to the image
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Something like

"""
Generate the 8 naive rotations of an image.

This can be helpful for testing, and for image algorithms that don't have
a native way to generate rotations during hashing, this can be a way to
brute force rotations. 

@haianhng31
Copy link
Contributor Author

@Dcallies Thank you for your comments! I have included the docstring where necessary and also tested it on my development environment. If the code is ok, I can start moving to next step which is adding a new command line option (--rotations)

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

So close!

Can you:

  1. Run black so CI passes
  2. Confirm that you did something like the test plan?

python-threatexchange/threatexchange/content_type/photo.py Outdated Show resolved Hide resolved
@haianhng31
Copy link
Contributor Author

haianhng31 commented Oct 30, 2024

Yes I have done the test plan and update the PR's description

@haianhng31 haianhng31 changed the title Create rotation helper functions in PhotoContent using Pillow [py-tx] Create rotation helper functions in PhotoContent using Pillow Oct 30, 2024
@haianhng31
Copy link
Contributor Author

@Dcallies it seems like StrEnum cannot be imported with py version before 3.11
Q: Would you like me to remove 3.9 from the workflow? Or would you want me to not use StrEnum and do something else instead?

@Dcallies
Copy link
Contributor

Oh no! I totally missed that.

For now, use Enum, and then type out the names of the values that would be equivalent to auto(). It should just be lowercase

@haianhng31
Copy link
Contributor Author

@Dcallies Fixed! Lmk if you have any other comment

@Dcallies Dcallies self-requested a review November 1, 2024 15:35
Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

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

Nice job!

@@ -7,6 +7,7 @@
This records all the valid signal types for a piece of content.
"""

from enum import Enum, auto
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: auto unused

@Dcallies Dcallies merged commit ce85474 into facebook:main Nov 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants