-
Notifications
You must be signed in to change notification settings - Fork 6k
[bug] Precedence of operations in VAE should be slicing -> tiling #9342
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
55a1abd
bugfix: precedence of operations should be slicing -> tiling
a-r-r-o-w af060b7
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w 93e4d23
fix typo
a-r-r-o-w 5ac6473
fix another typo
a-r-r-o-w 47d34c3
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w 4e2df5c
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w be9797a
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w ea25b69
deprecate current implementation of tiled_encode and use new impl
a-r-r-o-w 170d2d2
Merge branch 'main' into vae/bugfix-slicing-tiling
sayakpaul 0206822
Update src/diffusers/models/autoencoders/autoencoder_kl.py
a-r-r-o-w 896c8f7
Update src/diffusers/models/autoencoders/autoencoder_kl.py
a-r-r-o-w 1c02d76
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w 2bc58fa
Merge branch 'main' into vae/bugfix-slicing-tiling
a-r-r-o-w File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think algorithem changed a bit for use_slicing
previously, apply quant_conv once after combining encoder outputs from all slice
currently, apply quant_conv on each slice
I'm pretty sure the result would be the same, I wonder if there is any implication on performance?
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 think the performance should be the same since just one convolution layer on compressed outputs of encoder. I can get some numbers soon
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.
We could perhaps add a test to ensure this? That should clear the confusions?
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.
@a-r-r-o-w do you think it could make sense add a fast test here or not really?
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 think it's okay without a test here. The functionality is effectively similar and only affects the "batch_size" dim across this conv layer - which will never alter outputs as conv doesn't operate on that.
I know that understanding the changes here is quite easy, but I feel I should leave a comment making the explanation a bit more clear and elaborate for anyone stumbling upon this in the future.
Previously, slicing worked individually and tiling worked individually. When both were enabled, only tiling would be in effect meaning it would chop
[B, C, H, W]
to 4 tiles of shape[B, C, H // 2, W // 2]
(assuming we have 2x2 perfect tiles), process each tile individually and perform blending.This is incorrect as slicing is completely ignored. The correct processing size, ensuring slicing also took effect, would be 4 x B tiles with shape
[1, C, H // 2, W // 2]
- which this PR does.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.
Thanks for explaining!