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

instruct pix2pix pipeline: remove sigma scaling when computing classifier free guidance #7006

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

erliding
Copy link
Contributor

@erliding erliding commented Feb 18, 2024

Fixes (#6981)

instruct pix2pix pipeline scale model prediction to sigma space before computing classifier free guidance and scale back when done for schedulers operating in sigma space, which seems unnecessary, especially when rescale_betas_zero_snr is enabled for those schedulers, this scaling can result in fp16 overflow with high rates and generating black images

for example (noise_pred - latents) below can goes beyond -65504 and result in -inf for fp16 (for this specific case reorganize rhs of the assignment to latents / sigma - noise_pred / sigma should also work)

@yiyixuxu @sayakpaul

@sayakpaul
Copy link
Member

@erliding do you have some results for us to verify things here?

Cc: @patil-suraj here as well.

@yiyixuxu
Copy link
Collaborator

can you maybe run the docstring example with fixed seed from main branch and this branch and see if they are identical?
(I think math is the same)

@yiyixuxu yiyixuxu requested a review from patil-suraj February 18, 2024 07:44
@erliding
Copy link
Contributor Author

@erliding do you have some results for us to verify things here?

Cc: @patil-suraj here as well.

yeah, so i tested with EulerDiscreteScheduler and EulerAncestralDiscreteScheduler for both eps and v_prediction with StableDiffusionInstructPix2PixPipeline, removing sigma scaling doesn't seem to have noticeable impact on generated images, and setting rescale_betas_zero_snr=True won't cause generating black images for fp16 inference anymore

@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.

@erliding
Copy link
Contributor Author

i ran the example below on main branch and this branch, it use EulerAncestralDiscreteScheduler
https://github.com/huggingface/diffusers/tree/main/examples/instruct_pix2pix#inference
attached the edited images, first from main, second from this branch
edited_image-main
edited_image-branch

@sayakpaul
Copy link
Member

Alright, thanks. But I varely see any qualitative difference though.

@erliding
Copy link
Contributor Author

Alright, thanks. But I varely see any qualitative difference though.

it shouldn't, it is to show sigma scaling can be removed without introducing difference

@sayakpaul
Copy link
Member

Alright, yes, that makes sense!

@sayakpaul
Copy link
Member

I am okay with the changes but @patil-suraj needs to approve this.

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Mar 9, 2024

gentle pin @patil-suraj

Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

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

Great find! This was done to mimic the original code but actually shouldn't make difference here. Thanks a lot for the PR.

@patil-suraj patil-suraj merged commit a1cb106 into huggingface:main Mar 11, 2024
15 checks passed
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