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

Fix scheduler inference steps error with power of 3 #466

Merged
merged 5 commits into from
Sep 13, 2022

Conversation

natolambert
Copy link
Contributor

@natolambert natolambert commented Sep 10, 2022

Discussing in #444.
More verification to do on the unclear number of iterations vs num_inference_steps

Solution inspired by this discussion in original SD repo, thanks @ryanirl.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 10, 2022

The documentation is not available anymore as the PR was closed or merged.

@natolambert natolambert changed the title [WIP] Fix -- Scheduler inference steps error with power of 3 Fix scheduler inference steps error with power of 3 Sep 11, 2022
@patrickvonplaten
Copy link
Contributor

Cool that looks good to me! Thanks a lot for the fix. @natolambert could you also add one test that shows that it can now be a power of three?

@natolambert
Copy link
Contributor Author

natolambert commented Sep 12, 2022

Okay @patrickvonplaten, so this test should cover the test needed for now.
Note that the DDIMScheduler did not error out with powers of 3, but rather it had a number of iterations that did not match the expected behavior.

If we update DDIMScheduler to have the same alpha logic as PNDMScheduler, we should add the power of 3 test!

NOTE: I added a small fix where the DDIMSchedulerTest::test_inference_steps was not passing the time_step to check_over_forward like the other scheduler tests.

@pcuenca
Copy link
Member

pcuenca commented Sep 12, 2022

We need to verify this in the Flax implementation too; it probably happens there as well: https://github.com/patil-suraj/stable-diffusion-jax/blob/main/stable_diffusion_jax/scheduling_pndm.py#L103-L105

/cc @kashif

@patrickvonplaten
Copy link
Contributor

PR looks great to me!

@natolambert
Copy link
Contributor Author

I'll add a comment on the test explaining why it is there, then merge (otherwise it may be a little confusing to a reader). Great!

Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

LGTM, but there might be a case where we need to .round() for mathematical correctness, instead of just truncating the decimal with .astype(int). So watch out for those issues :)

@natolambert
Copy link
Contributor Author

@anton-l that's a good point. I'm going to check with round, I like that better (it's also less opaque).

@natolambert natolambert merged commit b56f102 into main Sep 13, 2022
@kashif kashif deleted the scheduler_inference_bug branch September 16, 2022 15:48
PhaneeshB pushed a commit to nod-ai/diffusers that referenced this pull request Mar 1, 2023
* [WEB] CSS changes to the web-ui (huggingface#465)

This commit updates UI with styling.

Signed-Off-by: Gaurav Shukla <[email protected]>

Signed-off-by: Gaurav Shukla <[email protected]>

* [WEB] Update the title (huggingface#466)

* [WEB] Add support for long prompts (huggingface#467)

* [WEB] fix background color

Signed-Off-by: Gaurav Shukla

* [WEB] Remove long prompts support

It removes support to long prompts due to higher lag in loading long prompts.

Signed-Off-by: Gaurav Shukla <gaurav@nod-labs>

* [WEB] Update nod logo and enable debug feature.

Signed-Off-by: Gaurav Shukla <[email protected]>

Signed-off-by: Gaurav Shukla <[email protected]>
Signed-off-by: Gaurav Shukla
Signed-off-by: Gaurav Shukla <gaurav@nod-labs>
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* initial attempt at solving

* fix pndm power of 3 inference_step

* add power of 3 test

* fix index in pndm test, remove ddim test

* add comments, change to round()
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