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

[WIP] Allow DDIMInverseScheduler to use same number of noising and denoising steps #3436

Closed

Conversation

kathath
Copy link
Contributor

@kathath kathath commented May 15, 2023

For the final DDIMScheduler step, we are already at t=0 and are predicting the sample at
t=0-num_train_steps/num_inference_steps.
The current DDIMInverseScheduler implemetation starts with the original image at t=0. But to match DDIMScheduler, one needs to start at t=0-num_train_steps/num_inference_steps in order to revert all denoising steps.
This can also be seen by the fact that there are num_inference_steps denoising steps but only num_inference_steps-1 noising steps in StableDiffusionPix2PixZeroPipeline and StableDiffusionDiffEditPipeline (those two pipelines are currently using DDIMInverseScheduler).

In contrast to the current DDIMInverseScheduler implementation, in Null-text Inversion for Editing Real Images using Guided Diffusion Models inversion starts at t=0-num_train_steps/num_inference_steps and reverses all denoising steps.
In the description of DDIMInverseScheduler, it is stated that "The implementation is mostly based on the DDIM inversion definition of Null-text Inversion for Editing Real Images using Guided Diffusion Models" - which is currently not quite the case.

This PR allows to use DDIM inversion as implemented in Null-text Inversion for Editing Real Images using Guided Diffusion Models, when setting revert_all_steps=True.

This PR is strongly influenced by https://github.com/google/prompt-to-prompt/#null-text-inversion-for-editing-real-images.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@patrickvonplaten
Copy link
Contributor

Hmm @clarencechen wdyt here? It essentially reverts the PR here: #2619

@clarencechen
Copy link
Contributor

@patrickvonplaten I wouldn't say it reverts #2619, since the DDIMInverseScheduler implementation before then was also dropping the last (most noisy) timestep, but it does revert the semantic interpretation of the timestamp argument away from "timestamp corresponding to current noise level of input", which I personally strongly prefer. I think I would wait until a fine-tuned model addressing #3475 is out, since the current timestep I'm skipping corresponds to the final steps_offset training timesteps of the DDIMScheduler denoising process.

@kathath kathath changed the title Allow DDIMInverseScheduler to use same number of noising and denoising steps [WIP] Allow DDIMInverseScheduler to use same number of noising and denoising steps May 30, 2023
@bghira
Copy link
Contributor

bghira commented May 30, 2023

i have uploaded ptx0/pseudo-journey-v2 which is trained using a patched noise schedule. it has 30,000 steps applied over most of the text encoder for SD 2.1-v over about 30,000 images.

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Left some comments inline

each diffusion step uses the value of alphas product at that step and at the previous one. For the final
step there is no previous alpha. When this option is `True` the previous alpha product is fixed to `0`,
otherwise it uses the value of alpha at step `num_train_timesteps - 1`.
set_alpha_to_one (`bool`, default `True`):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to do the renaming here? This is backwards breaking IMO

steps_offset: int = 0,
prediction_type: str = "epsilon",
clip_sample_range: float = 1.0,
revert_all_steps: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this ever used?

@github-actions
Copy link

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 github-actions bot added the stale Issues that haven't received updates label Jun 27, 2023
@github-actions github-actions bot closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues that haven't received updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants