diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

RePaint pipeline and stable-diffusion-inpaint pipeline legacy off-by-one error

Open Randolph-zeng opened this issue 2 years ago • 4 comments

Describe the bug

When I am using diffuser library to merge repaint pipeline logic with the SD-inpaint-pipeline-legacy, I observed an obvious performance drop which is very curious to me. I believe it is caused by two off-by-one errors.

The repaint paper introduced a pipeline to merge the known part of the image and unknown part of the image together. However, I believe the original paper might have some typo or error in its algorithm section. The same error I believe is also carried over to the diffuser implementation:

Screenshot 2022-12-07 at 14 31 53

The known part of x_{t-1} should be sqrt_alpha_bar_{t-1} * x_0 + sqrt_one_minus_alpha_bar_{t-1} * epsilon. There are two errors in original paper in line 5 of Algorithm 1. First the coefficient of epsilon is not sqrt-ed, second the timestep is off by one. Though the implementation of diffusers lib in https://github.com/huggingface/diffusers/blob/main/src/diffusers/schedulers/scheduling_repaint.py#L290 fixed the first error, I believe the second error is not fixed.

Another related and similar erorr is in SD-pipleine-inpaint-legacy: https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion_inpaint_legacy.py#L566 I see no reason to merge a latents from t-1 timestep with latents from timeste t.

I might be wrong that this is not a mismatch and is actually an intended design. Please kindly let me know if I am wrong. If this two mismatch is actually unwanted, I have provided two naive fix unless more sophiscated test suites is required. They can be found in https://github.com/huggingface/diffusers/pull/1583, https://github.com/huggingface/diffusers/pull/1582 Thanks : )

Reproduction

N/A

Logs

No response

System Info

N/A

Randolph-zeng avatar Dec 07 '22 06:12 Randolph-zeng

"the coefficient of epsilon is not sqrt-ed":

It is in the original repo: this line But you are right the coefficient square root is missing from the paper itself.

majedelhelou avatar Dec 07 '22 14:12 majedelhelou

"the coefficient of epsilon is not sqrt-ed":

It is in the original repo: this line But you are right the coefficient square root is missing from the paper itself.

How about the off by one timestep part tho, is it intended or maybe a small negligence ?

Randolph-zeng avatar Dec 08 '22 01:12 Randolph-zeng

cc @anton-l for RePaint

Regarding the legacy inpainting pipeline note that there is no official reference implementation of the legacy inpainting pipeline and instead we strongly recommend using the "correct" StableDiffusionInpaintPipeline

patrickvonplaten avatar Dec 12 '22 13:12 patrickvonplaten

:heavy_check_mark: RePaint updated here: https://github.com/huggingface/diffusers/pull/1582 For inpainting, let's continue the discussion in https://github.com/huggingface/diffusers/pull/1583 , haven't looked into it yet

anton-l avatar Dec 12 '22 14:12 anton-l

How about the off by one timestep part tho, is it intended or maybe a small negligence ?

Yes I also spotted some off by one errors, sorry for the late reply

majedelhelou avatar Jan 06 '23 16:01 majedelhelou

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

github-actions[bot] avatar Jan 31 '23 15:01 github-actions[bot]