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

add the option of upsample function for tiny vae #7604

Merged
merged 5 commits into from
Apr 10, 2024

Conversation

IDKiro
Copy link
Contributor

@IDKiro IDKiro commented Apr 8, 2024

What does this PR do?

Fixes # (issue)

  1. Provide other options for the upsampling function of the Tiny VAE's decoder (default is nearest), which will be used in SDXS.

Before submitting

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.

@yiyixuxu yiyixuxu requested a review from sayakpaul April 8, 2024 18:33
@HuggingFaceDocBuilderDev

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.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am cool with it. Out of curiosity: did you benefit from using a non-default upsampler?

@@ -926,6 +926,7 @@ def __init__(
block_out_channels: Tuple[int, ...],
upsampling_scaling_factor: int,
act_fn: str,
upsample_fn: str,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have the default of Upsample here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is not necessary. I checked the code, DecoderTiny will only be called when AutoencoderTiny is initialized, and upsample_fn is bound to pass parameters, consistent with act_fn. The default value of nn.Upsample is "nearest", which upsample_fn will pass. So the change will not affect the original TAESD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

@sayakpaul sayakpaul requested a review from yiyixuxu April 9, 2024 01:33
@IDKiro
Copy link
Contributor Author

IDKiro commented Apr 9, 2024

I am cool with it. Out of curiosity: did you benefit from using a non-default upsampler?

We found that using the default nearest upsampler tends to lead to lattice-like artifacts. Although the lattice artifacts can be mitigated by increasing the weights of the GAN loss, it still leads to other texture artifacts in the generated images. The problem is significantly mitigated when the bilinear upsampler is enabled. The problem was discovered when we performed face restoration using Tiny VAE.

Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! thank you!

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Apr 9, 2024

would be nice to see an comparison so that we can add a note to our doc
would merge this regardless

@sayakpaul
Copy link
Member

@IDKiro once my comments are addressed, happy to merge the PR.

@IDKiro
Copy link
Contributor Author

IDKiro commented Apr 10, 2024

@IDKiro once my comments are addressed, happy to merge the PR.

Demonstrate a special sample (most samples require a closer look) where images are generated using the model.
Use nearest:
image
Using bilinear:
image

@sayakpaul
Copy link
Member

Just have a single comment to be addressed and then we're good to go: #7604 (comment).

@sayakpaul sayakpaul merged commit b99b161 into huggingface:main Apr 10, 2024
15 checks passed
@redouanelg
Copy link

It would be nice to be able to choose this possibility for AutoencoderKL too

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 this pull request may close these issues.

5 participants