-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Make T2I-Adapter downscale padding match the UNet #5435
Make T2I-Adapter downscale padding match the UNet #5435
Conversation
…t and width as params.
…UNet down blocks so that all T2I-Adapter down blocks get exercised.
@@ -399,7 +409,7 @@ def __init__(self, in_channels: int, out_channels: int, num_res_blocks: int, dow | |||
|
|||
self.downsample = None | |||
if down: | |||
self.downsample = Downsample2D(in_channels) | |||
self.downsample = nn.AvgPool2d(kernel_size=2, stride=2, ceil_mode=True) |
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.
Note to reviewers:
Downsample2D(in_channels)
is really just a nn.AvgPool2d(kernel_size=2, stride=2)
layer. The only real change here is setting ceil_mode=True
.
expected_slice = np.array( | ||
[0.27978146, 0.36439905, 0.3206715, 0.29253614, 0.36390454, 0.3165658, 0.4384598, 0.43083128, 0.38120443] | ||
) |
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.
Note to reviewers:
You'll notice several expected_slice
values changing in this PR. These values changed as a result of changes to get_dummy_components(...)
in 6d4a060. The changes to adapter.py did not require changes to expected_slice
. I was careful to keep these changes in separate commits so that you can verify this, if desired.
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 still prefer to keep the default tests unchanged and instead add new tests. If possible, could we do that? Applies to all the tests below.
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.
Done. See my comment here: #5435 (comment)
(My original comment in this thread no longer applies.)
src/diffusers/pipelines/t2i_adapter/pipeline_stable_diffusion_adapter.py
Show resolved
Hide resolved
tests/pipelines/stable_diffusion/test_stable_diffusion_adapter.py
Outdated
Show resolved
Hide resolved
tests/pipelines/stable_diffusion/test_stable_diffusion_adapter.py
Outdated
Show resolved
Hide resolved
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.
The changes look very clean to me. Thanks for keeping them that way!
I have some concerns regarding the changes made to the default tests. Otherwise, the PR is already in a very good state!
…mber of UNet down blocks so that all T2I-Adapter down blocks get exercised." This reverts commit 6d4a060.
…ating them dynamically.
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.
great! thank you!
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.
Very comprehensive and clean PR. Thank you for your hard work!
I wonder if something about the original issue underlying this PR should be included in our docs in the interest of full transparency. WDYT?
@RyanJDick you'd need to run: make style && make quality
make fix-copies to mitigate the failing of the style and repo consistency tests. |
Fixed style checks and noted for future.
Good question. I feel like its lower-level than most people would be interested in, and lower-level than the rest of the documentation. It might just add confusion. What do you think? |
Failing test related to consistency is unrelated. |
Very nice PR! |
* Update get_dummy_inputs(...) in T2I-Adapter tests to take image height and width as params. * Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised. * Update the T2I-Adapter down blocks to better match the padding behavior of the UNet. * Revert "Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised." This reverts commit 6d4a060. * Create utility functions for testing the T2I-Adapter downscaling bahevior. * (minor) Improve readability with an intermediate named variable. * Statically parameterize T2I-Adapter test dimensions rather than generating them dynamically. * Fix static checks. --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
* Update get_dummy_inputs(...) in T2I-Adapter tests to take image height and width as params. * Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised. * Update the T2I-Adapter down blocks to better match the padding behavior of the UNet. * Revert "Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised." This reverts commit 6d4a060. * Create utility functions for testing the T2I-Adapter downscaling bahevior. * (minor) Improve readability with an intermediate named variable. * Statically parameterize T2I-Adapter test dimensions rather than generating them dynamically. * Fix static checks. --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
* Update get_dummy_inputs(...) in T2I-Adapter tests to take image height and width as params. * Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised. * Update the T2I-Adapter down blocks to better match the padding behavior of the UNet. * Revert "Update the T2I-Adapter unit tests to run with the standard number of UNet down blocks so that all T2I-Adapter down blocks get exercised." This reverts commit 6d4a060. * Create utility functions for testing the T2I-Adapter downscaling bahevior. * (minor) Improve readability with an intermediate named variable. * Statically parameterize T2I-Adapter test dimensions rather than generating them dynamically. * Fix static checks. --------- Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
What does this PR do?
Fixes #5377
This PR updates the padding behavior of the T2I-Adapter down blocks to more closely mirror the padding behavior of the UNet model.
Prior to this change, the T2I-Adapter pipelines only worked with input/output image dimensions that were multiples of the T2I-Adapter's total downscale factor (64 for SD1 and 32 for SDXL). After this change, the same pipelines can be run with input/output dimensions that are multiples of the T2I-Adapter's initial pixel unshuffling downscale factor (8 for SD1 and 16 for SDXL).
Manual Testing
The change is covered by unit tests, but the following script can be used as a visual regression test for this feature. This script is adapted from this comment on the original issue.
Input
dog.png
:Before this change,
in_dims=(680, 384)
Note that output dimensions do not match input dimensions.
In dims: (680, 384), Out dims: (640, 384)
Before this change,
in_dims=(640, 384)
In dims: (640, 384), Out dims: (640, 384)
After this change,
in_dims=(680, 384)
The output dimensions match the input dimensions, as expected. The change in the output's appearance is expected, because the latent dimension has changed, so the same random seed no longer produces the same noise.
In dims: (680, 384), Out dims: (680, 384)
After this change,
in_dims=(640, 384)
The output image for dimensions that already 'worked' is identical to before the change.
In dims: (640, 384), Out dims: (640, 384)
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.