[Bug fix] "previous_timestep()" in DDPM scheduling compatible with "trailing" and "linspace" options

[AnandK27]

What does this PR do?

Fixes the bug of previous_timestep() not giving the right timestep for numbers not a factor of 1000 during "trailing" and "linspace" options. The previous_timestep has to be accruate for the options only during inference and since during inference the timesteps array is available, I just grab the value from there as for custom_timesteps. During training it is going to be 1 less than the current timestep.

(This is simplest solution I can do, if you need calculating the previous timestep for each case without the timesteps array let me know!)

Test Script

import torch
from diffusers import DDPMScheduler

# Initialize the DDPM scheduler
scheduler = DDPMScheduler.from_pretrained("google/ddpm-celebahq-256")

#test params
scheduler.config.timestep_spacing = 'trailing' # or 'linspace'
scheduler.set_timesteps(num_inference_steps = 7) # any prime number

# Set up a dummy pipeline
generator = torch.manual_seed(0)

# Initialize some dummy latents (normally this would come from your model)
latents = torch.randn((1, 3, 64, 64))

# Inference loop
for i, t in enumerate(scheduler.timesteps):
    # In a real scenario, you'd run your model here to get model_output
    model_output = torch.randn_like(latents)
    
    # Get the previous timestep
    prev_timestep = scheduler.previous_timestep(t)
    
    # Print current and previous timesteps
    print(f"Step {i}:")
    print(f"  Current timestep: {t}")
    print(f"  Previous timestep: {prev_timestep}")
    
    # Scheduler step
    latents = scheduler.step(model_output, t, latents, generator=generator).prev_sample

print("Test complete.")

Output

Step 0:
  Current timestep: 999
  Previous timestep: 856
Step 1:
  Current timestep: 856
  Previous timestep: 713
Step 2:
  Current timestep: 713
  Previous timestep: 570
Step 3:
  Current timestep: 570
  Previous timestep: 428
Step 4:
  Current timestep: 428
  Previous timestep: 285
Step 5:
  Current timestep: 285
  Previous timestep: 142
Step 6:
  Current timestep: 142
  Previous timestep: -1
Test complete.

Fixes #9261

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline?
• Did you read our philosophy doc (important for complex PRs)?
• Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

@yiyixuxu @RochMollero @bghira
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[RochMollero]

I think it indeed fixes the issue . That said: - it does not fix ddim or any other scheduler that are likely to have the same mistake such as pndm. - the current code around schedulers feel generally too complicated and duplicated across schedulers. Would you guys be interested in simplifying (ddim and other could inherit some functions from ddpm) and / or having some test framework ? (For another PR maybe) Otherwise its good to me for this PR

…

________________________________ De : Anand Kumar ***@***.***> Envoyé : samedi 7 septembre 2024 09:22 À : huggingface/diffusers ***@***.***> Cc : RochMollero ***@***.***>; Mention ***@***.***> Objet : [huggingface/diffusers] [Bug fix] "previous_timestep()" in DDPM scheduling compatible with "trailing" and "linspace" options (PR #9384) What does this PR do? Fixes the bug of previous_timestep() not giving the right timestep for numbers not a factor of 1000 during "trailing" and "linspace" options. The previous_timestep has to be accruate for the options only during inference and since during inference the timesteps array is available, I just grab the value from there as for custom_timesteps. During training it is going to be 1 less than the current timestep. (This is simplest solution I can do, if you need calculating the previous timestep for each case without the timesteps array let me know!) Test Script import torch from diffusers import DDPMScheduler # Initialize the DDPM scheduler scheduler = DDPMScheduler.from_pretrained("google/ddpm-celebahq-256") #test params scheduler.config.timestep_spacing = 'trailing' # or 'linspace' scheduler.set_timesteps(num_inference_steps = 7) # any prime number # Set up a dummy pipeline generator = torch.manual_seed(0) # Initialize some dummy latents (normally this would come from your model) latents = torch.randn((1, 3, 64, 64)) # Inference loop for i, t in enumerate(scheduler.timesteps): # In a real scenario, you'd run your model here to get model_output model_output = torch.randn_like(latents) # Get the previous timestep prev_timestep = scheduler.previous_timestep(t) # Print current and previous timesteps print(f"Step {i}:") print(f" Current timestep: {t}") print(f" Previous timestep: {prev_timestep}") # Scheduler step latents = scheduler.step(model_output, t, latents, generator=generator).prev_sample print("Test complete.") Output Step 0: Current timestep: 999 Previous timestep: 856 Step 1: Current timestep: 856 Previous timestep: 713 Step 2: Current timestep: 713 Previous timestep: 570 Step 3: Current timestep: 570 Previous timestep: 428 Step 4: Current timestep: 428 Previous timestep: 285 Step 5: Current timestep: 285 Previous timestep: 142 Step 6: Current timestep: 142 Previous timestep: -1 Test complete. Fixes #9261<#9261> Before submitting * This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case). * Did you read the contributor guideline<https://github.com/huggingface/diffusers/blob/main/CONTRIBUTING.md>? * Did you read our philosophy doc<https://github.com/huggingface/diffusers/blob/main/PHILOSOPHY.md> (important for complex PRs)? * Was this discussed/approved via a GitHub issue or the forum<https://discuss.huggingface.co/c/discussion-related-to-httpsgithubcomhuggingfacediffusers/63>? Please add a link to it if that's the case. * Did you make sure to update the documentation with your changes? Here are the documentation guidelines<https://github.com/huggingface/diffusers/tree/main/docs>, and here are tips on formatting docstrings<https://github.com/huggingface/diffusers/tree/main/docs#writing-source-documentation>. * Did you write any new necessary tests? Who can review? @yiyixuxu<https://github.com/yiyixuxu> @RochMollero<https://github.com/RochMollero> @bghira<https://github.com/bghira> Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.

________________________________ You can view, comment on, or merge this pull request online at: #9384 Commit Summary * d1f764a<d1f764a> Update scheduling_ddpm.py File Changes (1 file<https://github.com/huggingface/diffusers/pull/9384/files>) * M src/diffusers/schedulers/scheduling_ddpm.py<https://github.com/huggingface/diffusers/pull/9384/files#diff-5014bf116e54d20c980d805dc58ac83ac75b2976a2d785978632f14d8fb53ea9> (8) Patch Links: * https://github.com/huggingface/diffusers/pull/9384.patch * https://github.com/huggingface/diffusers/pull/9384.diff — Reply to this email directly, view it on GitHub<#9384>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA374DJHQLB4JB2WY5LDVJTZVKSUXAVCNFSM6AAAAABNZZCRSOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGUYTCNJRG4YTAOI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[AnandK27]

Yeah the change has to be propagated to remaining schedulers. There is also a lot of duplication in schedulers and will need an overhaul to modularize it. Probably the moderators can open issues for these.

[yiyixuxu]

thanks so much for your PR! will take a look!
all our newer schedulers has a step counter so would not have this issue

diffusers/src/diffusers/schedulers/scheduling_dpmsolver_multistep.py

Line 283 in d08ad65

def step_index(self):

[AnandK27]

I will close the PR then

[yiyixuxu]

hey! sorry I just noticed this
it appears my comment #9384 (comment) has been misleading, I only meant that we do not need to apply the change for remaining schedulers (not all of them anyway), this is still very much an issue with DDPM

feel free to re-open this

[github-actions]

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.

[yiyixuxu]

can you take a look? @hlky

[HuggingFaceDocBuilderDev]

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.

[hlky]

Thanks @AnandK27! 🤗