Skip to content

Derive DDIM prev_timestep from the inference schedule - #14357

Open
MohammadSadeghSalehi wants to merge 2 commits into
huggingface:mainfrom
MohammadSadeghSalehi:prev-timestep-schedule-mismatch
Open

Derive DDIM prev_timestep from the inference schedule#14357
MohammadSadeghSalehi wants to merge 2 commits into
huggingface:mainfrom
MohammadSadeghSalehi:prev-timestep-schedule-mismatch

Conversation

@MohammadSadeghSalehi

Copy link
Copy Markdown
Contributor

Summary

DDIMScheduler.step formed the previous diffusion index with a uniform stride:

prev_timestep = timestep - self.config.num_train_timesteps // self.num_inference_steps

That expression assumes every inference step advances by the same integer. set_timesteps with timestep_spacing="linspace" does not produce that schedule. For 1000 train steps and 10 inference steps the materialised list strides by 111 while the expression strides by 100, so every step reads alpha_prod_t_prev from the wrong index.

DDIMScheduler defaults to timestep_spacing="leading". Only callers that explicitly select linspace change behaviour. Default sampling outputs do not move.

Proof that leading and trailing are no-ops

Full deterministic loops (eta=0) comparing schedule lookup against the historical stride, same residuals:

spacing bit-identical max abs diff
leading True 0.000000e+00
trailing True 0.000000e+00
linspace False 2.584383e+02

On leading and trailing, intermediate prev values match the old stride exactly. At the final step the formula can yield a large negative (for example -100) while the list yields -1; both are < 0 and both select final_alpha_cumprod, so the sample is still bit-identical. On linspace, 9 of 10 steps change alpha_prod_t_prev. Example prev sequence for 1000/10 linspace: schedule [888, 777, …, 0, -1] versus hardcoded [899, 788, …, 11, -100].

Change

Add previous_timestep() that returns the next entry of self.timesteps (last entry → -1). Use it from step (and the parallel batch / variance paths). Same helper is # Copied from into the three DDIM-family schedulers below; make fix-copies is clean.

Scope: four changed, two deliberately not

Changed

Scheduler Why
DDIMScheduler Source of the bug in #12633 / #11347
DDIMParallelScheduler Same formula in step, _get_variance, and batch path
CogVideoXDDIMScheduler Same formula in step
CogVideoXDPMScheduler Same formula in step

None of those step methods was a # Copied from of another; the formula was independently duplicated. The new helper is the shared copy point.

Not changed

Scheduler Why list lookup is wrong there
PNDMScheduler step_prk uses half-stride intermediates that are not entries of self.timesteps. step_plms rewrites prev_timestep and timestep when counter == 1. With skip_prk_steps, the list contains deliberate duplicates, so a first-match index would pick the wrong row.
RePaintScheduler The schedule contains forward jumps. The next list entry is often a larger t, while the stride formula always means one reverse diffusion step. List lookup would break jump steps.

Off-schedule fallback

When step is called with a timestep that is not an entry of self.timesteps, previous_timestep falls back to the historical stride. That is not a defensive guess: replacing the fallback with a ValueError fails five existing tests that call step() outside the inference loop:

  • test_eta
  • test_inference_steps
  • test_steps_offset
  • test_time_indices
  • test_timestep_spacing

Pipeline loops always pass schedule entries, so the linspace fix is not diluted by the fallback. Happy to convert the fallback to a raise and update those five tests if maintainers prefer a stricter API.

Issues

Closes #12633 and #11347. They are one root cause (stride vs linspace schedule), not two independent bugs.

Test plan

  • test_prev_timestep_matches_schedule fails at pristine base 4b8e466bf and passes here
  • Leading / trailing full-loop bit-identical to the old stride (max_abs_diff=0)
  • Linspace prev sequence equals the schedule; trajectory changes
  • tests/schedulers/test_scheduler_ddim.py and test_scheduler_ddim_parallel.py: 78 passed
  • make style exit 0 (ruff + doc-builder + extra checks)
  • make fix-copies exit 0

Platform: macOS arm64, CPU, CPython 3.12.


Self-review

Scope

Branch prev-timestep-schedule-mismatch against base 4b8e466bf.

Files:

  • src/diffusers/schedulers/scheduling_ddim.py
  • src/diffusers/schedulers/scheduling_ddim_parallel.py
  • src/diffusers/schedulers/scheduling_ddim_cogvideox.py
  • src/diffusers/schedulers/scheduling_dpm_cogvideox.py
  • tests/schedulers/test_scheduler_ddim.py

Blocking issues

None.

Non-blocking issues

  1. PNDM and RePaint still use the stride formula

    • Same line text appears, but list lookup is wrong for PLMS/PRK half-steps and RePaint jumps.
    • Leave for a follow-up if maintainers want a different design there.
  2. Off-schedule step() fallback

    • Load-bearing for the five tests named above. Alternative is raise + update those tests; offered in the PR body.

Dead code

None introduced.

Copied-from

Block Action
DDIMScheduler.previous_timestep New source method
Parallel / CogVideoX DDIM / CogVideoX DPM previous_timestep # Copied from DDIM; make fix-copies clean
step bodies Not copies; each call site updated to use previous_timestep

Evidence

  • test_prev_timestep_matches_schedule fails at base (AttributeError: previous_timestep) and passes here.
  • Leading / trailing full-loop bit-identical (max_abs_diff=0); linspace changes (max_abs_diff≈2.58e2).
  • DDIM default remains timestep_spacing="leading".
  • make style and make fix-copies exit 0 on this branch (pre-existing tree-wide reformat noise from ruff format / doc-builder on unrelated files was not committed).

Verdict

READY.

DDIMScheduler.step used a uniform stride,
num_train_timesteps // num_inference_steps, to form prev_timestep. That
matches leading and trailing spacing but not linspace: for 1000 train
steps and 10 inference steps the schedule steps by 111 while the formula
steps by 100, so alpha_prod_t_prev is read from the wrong index.

Look up the previous value as the next entry of self.timesteps, as
DDPMScheduler.previous_timestep already does. Leading and trailing stay
bit-identical: intermediate prev values match the old stride, and the
final step still selects final_alpha_cumprod whether the formula yields
-100 or the list yields -1. Default spacing remains leading, so only
callers that set linspace change.

Apply the same helper to DDIMParallel, CogVideoXDDIM and CogVideoXDPM
via # Copied from. PNDM and RePaint keep the stride formula: PLMS/PRK
half-steps and RePaint jump schedules are not consecutive schedule
entries.

Fixes huggingface#12633
Fixes huggingface#11347
doc-builder style rewrites these four copied docstrings to the project
max length of 119. Keep the copies in sync with make fix-copies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DDIMScheduler.step() logic mismatches set_timesteps() list when using timestep_spacing='linspace'

1 participant