-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement SharpBilinear scaler #102
base: master
Are you sure you want to change the base?
Conversation
fac9701
to
d7f57a1
Compare
return Bilinear.scale(clip, target_width, target_height, shift, **scale_kwargs) | ||
|
||
max_ratio = max(target_width / clip.width, target_height / clip.height) | ||
int_ratio = 2 ** int(log2(max_ratio) + 0.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can I interest you in our newest invention, math.ceil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am die goodbye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it’s transpired that this was, of course, round
, not math.ceil
, oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, round isn't exactly the math round which can cause issue
Look here: https://realpython.com/python-rounding/#pythons-built-in-round-function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so it’s round-half-to-even? I didn’t know that. Thanks!
Anyway, @LightArrowsEXE, why is this forced to be a power of two? And do you actually want to overshoot the ratio and then downscale with bilinear, undershoot and then upscale with bilinear, or pick the closest integer in any direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, it is round-half-to-even.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astiob I noticed while writing this that sometimes, it looks better when you downscale it vs. upscale it. I found that was the case if you had for example a factor of 2.5, upscaling looks better, but if you have for example a factor of 3.5, downscaling looks better.
f1212cd
to
d7f57a1
Compare
Would like a bit more feedback on this before I merge into master.
Given how long this file is becoming, I'm also tempted to say we should split scalers up into separate .py files in a "scalers" subdir. Thoughts?