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

Clean Code for Blurring and Upscaling Process #17

Open
takeny1998 opened this issue Aug 11, 2023 · 2 comments · May be fixed by #18
Open

Clean Code for Blurring and Upscaling Process #17

takeny1998 opened this issue Aug 11, 2023 · 2 comments · May be fixed by #18

Comments

@takeny1998
Copy link
Contributor

takeny1998 commented Aug 11, 2023

I'd like to discuss about changing name of functions and variables related to the upscaling process.

  1. expand_image method in Painting class.
  • I think expand_image is a good to describe the behavior,
  • but the upscale_image looks better to express Upscaling Process.

  1. target_size parameter in expand_image method.
  • it can be mislead users think that they are changing image size as fixed size.
  • since we're scaling the image, upscaling_ratio seems right. and its type should be float.

  1. blurring method in Painting Class.
  • A function's name should clearly descible its behavior and its target.
  • I think blurring should include the target, so **blur_image is more correct.**

what do you think about those? I'd like to hear.

@Minku-Koo
Copy link
Member

Oh that is gooooood idea.

3 things that you suggest is nive opinion.

Can you help me? try change and PR to us.

Thank you :)

takeny1998 added a commit to takeny1998/pypipo that referenced this issue Aug 15, 2023
@takeny1998 takeny1998 linked a pull request Aug 15, 2023 that will close this issue
@takeny1998
Copy link
Contributor Author

resolved with PR #18

in addition, I'd like to discuss 2 things.

  1. remove is_upscale bool flag.

    • upscale_image has ratio parameter for now, and I handled that do not perform upscaling image if users sets it as 1.0.
    • so, why don't we remove is_upscale? then it'll makes user handles upscaling intense easier.
    • but, default value that is_upscale is false, so idk how do I set default upscaling_ratio value if is_upscale removed.
  2. auto calculation in upscaling.

    • to fit the image size to 5000 pixel, we are calculating ratio an integer value.
    • BUT, if the image size is 4999 pixel, the result will be 9998 pixel. is the calculation what you intended?
    • if not, just calculate it an float. it will be more accurate.

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants