-
Notifications
You must be signed in to change notification settings - Fork 6k
refactor DPMSolverMultistepScheduler using sigmas #4986
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
@@ -54,6 +54,7 @@ def check_over_configs(self, time_step=0, **config): | |||
|
|||
output, new_output = sample, sample | |||
for t in range(time_step, time_step + scheduler.config.solver_order + 1): | |||
t = scheduler.timesteps[t] |
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 used 0
,1
,2
directly as timestep
index here, but they are not even in the self.timestepes
. In the previous implementation, it will default to the last timesteps when it is outside of the timesteps range, but I don't think it is intended. I changed it here, I think it makes more sense this way. Let me know if it's not the case
@@ -241,11 +242,3 @@ def test_fp16_support(self): | |||
sample = scheduler.step(residual, t, sample).prev_sample | |||
|
|||
assert sample.dtype == torch.float16 | |||
|
|||
def test_unique_timesteps(self, **config): |
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.
don't need this test anymore because we allow duplicated timesteps now
@patrickvonplaten another review please :) |
for scheduler_class in self.scheduler_classes: | ||
scheduler_config = self.get_scheduler_config(**config) | ||
scheduler = scheduler_class(**scheduler_config) | ||
|
||
scheduler.set_timesteps(scheduler.config.num_train_timesteps) | ||
assert len(scheduler.timesteps.unique()) == scheduler.num_inference_steps | ||
assert len(scheduler.timesteps) == scheduler.num_inference_steps |
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.
nice!
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 nice!
@yiyixuxu This refactor has broken stable diffusion (1.5) img2img for both use denoising strength = 0.6 |
Same with DPMSolverMultistepScheduler using Karras sigmas. Just returns noise with anything under 1.0 denoising strength. Havent tested it without Karras sigmas. |
@ljk1291 |
it seems like this bug still resurfaces somehow when using the legacy inpainting pipeline for some reason |
Hi @burgalon can you be more specific? could you maybe open a bug report and provide an example? thanks! YiYi |
@yiyixuxu |
--------- Co-authored-by: yiyixuxu <yixu310@gmail,com> Co-authored-by: Patrick von Platen <[email protected]>
--------- Co-authored-by: yiyixuxu <yixu310@gmail,com> Co-authored-by: Patrick von Platen <[email protected]>
I think I overcomplicated things with this #4690 by trying to follow k-diffusion implementation.
trying it again with a simpler approach here: we simply calculate
lambda_t
,sigma_t
andalpha_t
fromsigma
to-do
testing
Notes:
I compared the results against the current implementation (for dpm multistep only). There is some slight numerical difference when using k-sigmas; for example, in the last testing examples below on the 4th row (
sde-dpmsolver++, use_karras_sigma=True
), you can visually see the outputs are slightly different.Also, I had to loosen a test here #4986 (comment).
However, the new implementation is more accurate by using the sigma directly.