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

Adding a bulk resize image function #224

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

amannaik247
Copy link

I have a updated bulk_ops_common file to define image resize function which is used in the resize_image.py file . I have also changed the test shells to be portable since it was creating an issue on my side. Please review the code and let me know. Thank you!

@Udayraj123
Copy link
Owner

Hey @amannaik247, thanks for raising the PR! This looks great. Will review in detail, just need one more thing - can you also check the scripts/hooks/resize_images_hook.py file and see if we can import your function in that hook to have the same functionality?

@Udayraj123
Copy link
Owner

Linking #213

Copy link
Owner

@Udayraj123 Udayraj123 left a comment

Choose a reason for hiding this comment

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

Looks good overall, please check comments on refactoring

:param trigger_size: Minimum file size in bytes to trigger resizing.
"""
# Extract image files from the input directory
file_extensions = ["jpg", "jpeg", "png", "tiff"]
Copy link
Owner

Choose a reason for hiding this comment

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

let's move this as an cli arg, and add this as the default value. Also add a validation that only a subset of these 4 values is supported

@@ -119,3 +121,38 @@ def run_argparser(argparser):
raise Exception(f"\nError: Unknown arguments: {unknown}")

return args


def resize_image(
Copy link
Owner

Choose a reason for hiding this comment

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

let's move this function into image_utils.py instead

Comment on lines +144 to +150
if max_width and width > max_width:
new_height = int((max_width / width) * height)
img = img.resize((max_width, new_height), Image.ANTIALIAS)

if max_height and height > max_height:
new_width = int((max_height / height) * width)
img = img.resize((new_width, max_height), Image.ANTIALIAS)
Copy link
Owner

Choose a reason for hiding this comment

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

I already have a util created for this in image_utils.py, can you check if we can use that?


if __name__ == "__main__":
# Set up argument parser and parse arguments
argparser = get_local_argparser()
Copy link
Owner

Choose a reason for hiding this comment

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

actually you should use add_common_args(...) which internally uses the get_local_argparser().

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.

2 participants