-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Add final_sigma_zero
to UniPCMultistep
#7517
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
Conversation
Effectively the same trick as DDIM's `set_alpha_to_one` and DPM's `final_sigma_type='zero'`. Currently False by default but maybe this should be 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.
ohh thanks for doing this!
could we do final_sigma_type='zero'|'sig_min'
like DPM scheduler? we always prefer the string type of argument because it is more flexible. e.g. right now we only have two options, but it will allow us to add more options in the future without increase the number of parameters
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. |
Should 1:1 match DPM Multistep now.
I updated it to use the same values as DPM Multistep, defaulting to "zero" and raising val err on unknown type. What other possible types might there be? I thought for a bit and nothing really made sense besides sigmas[-1] and zero. I guess you could try to follow the curve by interpolating sigmas [-2:] to create a new non-zero, but that'd still have some residual noise I'd think. Also if it's Optional[str] should we handle |
looks like some tests are failing because we changed the default
|
it is hard to predict what's possible in the future, so as a general rule we are trying to always use string type of argument whenever possible
we don't have to handle |
|
thank you! |
This reverts commit 7478f32. I fixed the noise issue upstream in huggingface/diffusers#7517 Which was just merged
* Add `final_sigma_zero` to UniPCMultistep Effectively the same trick as DDIM's `set_alpha_to_one` and DPM's `final_sigma_type='zero'`. Currently False by default but maybe this should be True? * `final_sigma_zero: bool` -> `final_sigmas_type: str` Should 1:1 match DPM Multistep now. * Set `final_sigmas_type='sigma_min'` in UniPC UTs
This reverts commit 0c1f156. I fixed the noise issue upstream in huggingface/diffusers#7517 Which was just merged
* Add `final_sigma_zero` to UniPCMultistep Effectively the same trick as DDIM's `set_alpha_to_one` and DPM's `final_sigma_type='zero'`. Currently False by default but maybe this should be True? * `final_sigma_zero: bool` -> `final_sigmas_type: str` Should 1:1 match DPM Multistep now. * Set `final_sigmas_type='sigma_min'` in UniPC UTs
Effectively the same trick as DDIM's
set_alpha_to_one
and DPM'sfinal_sigma_type='zero'
where the extra concatenated sigma is simply zero instead of the prior sigma. Completely fixes the goofy residual noise, particularly with lower step counts.1st and 2nd order UniPC without the patch


1st and 2nd order UniPC with
final_sigma_zero=True
Third order errors out for me even on a clean
main
branch so that's fun. I'll monkey with it later...Currently
False
by default but maybe this should beTrue
like DDIM/DPM?This could also be represented as the same
final_sigma_type='zero'|'sig_min'
that DPM uses, but I don't really see why it needs to be a string with only two options. Personally I find this scheme much more intuitive.No UT right now as I didn't see anything for
final_sigma_type='zero'
in DPM's UTs.@yiyixuxu