-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
🚨 Update image_processing_vitmatte.py #30566
Conversation
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.
Looks great - thanks for handling this!
Could you add a test?
pad_width = size_divisibility - width % size_divisibility | ||
pad_height = 0 if height % size_divisibility == 0 else size_divisibility - height % size_divisibility | ||
pad_width = 0 if width % size_divisibility == 0 else size_divisibility - width % size_divisibility | ||
if pad_width + pad_height > 0: |
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.
Nice :)
Test added! be4e44b |
@rb-synth Great! Last thing is to make sure the slow tests for the model still pass:
Once you've confirmed they're all passing (or not different from running on main) then we're good to merge! |
doesn't work locally for me (neither does
|
@rb-synth Interesting - it looks like there's an issue with test setup / env. Running Last night, we merge in a feature enabling us to run specific slow model tests on our github action runners. This is useful as sometimes there's differences which arise from hardward and environments. To enable this for the vitmatte tests could you:
|
Have done that now |
Also tried the testing on a different machine and it completed without any issues:
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
@rb-synth Great! The slow tests on the github action runners are also all passing; and the quality checks are unrelated to this PR so we can merge. I'm going to mark the PR title with an 🚨 - as there's a possibility this could cause downstream changes for users who were relying on the previous behaviour. As this padding behaviour is more correct, and slow model tests are all OK, I think it's OK. |
In the previous implementation, if either dimension wasn't divisible by factor, then both would be padded even if only one needed it.
For example, for divisible factor 32, if I have an image of size
(1024, 819)
, then I would expect a padded size of(1024, 832)
. However, with the current implementation, even though 1024 is divisible by 32, it will be padded by a full 32 pixels, and the output size is(1056, 832)
.This PR fixes that 🐛 by computing dimensions individually.