Uh oh!
There was an error while loading. Please reload this page.
Use configured timesteps in DDPM parallel batch steps - #14692
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new schedule lookup can silently choose an incorrect prev_t for unexpected timesteps (no-match case), and the new test’s mock should be device/dtype/shape-aware to avoid brittleness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a schedule-parity bug in DDPMParallelScheduler.batch_step_no_noise() by deriving prev_t from the configured scheduler.timesteps (including linspace, trailing, and explicit custom timesteps) rather than using a fixed training-stride heuristic, and adds a regression test that checks batched vs scalar-step parity under deterministic (zero-noise) conditions.
Changes:
- Update
batch_step_no_noise()to computeprev_tvia the configured timestep schedule when running with inference/custom timesteps. - Add a parity test ensuring
batch_step_no_noise()matches repeated scalarstep()across multiple timestep schedules. - Add test-time mocking to force deterministic (zero) noise in the scalar path for comparison.
File summaries
| File | Description |
|---|---|
src/diffusers/schedulers/scheduling_ddpm_parallel.py | Updates batch_step_no_noise() to follow the configured timestep schedule when computing the previous timestep. |
tests/schedulers/test_scheduler_ddpm_parallel.py | Adds regression coverage validating batched no-noise stepping matches scalar stepping across multiple schedules. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Hi @Robertboy18, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Once the PR links an issue (or gets the |
2c98dbb to
ff22bebCompare
What does this PR do?
I was comparing the DDPM parallel batch path with the regular scheduler step and noticed that
batch_step_no_noise()still used a fixed training-step stride. This is the remaining batched case from #9261 (Fixes#9261); the scalar path was already fixed in #9384.For
linspace,trailing, and custom schedules, that could choose different alpha coefficients from the actual next configured timestep. This now followsscheduler.timestepsand adds parity tests against the scalar deterministic step :)I also have a small TorchLean formulation of the schedule-successor invariant and can attach it if useful.
Tests
pytest tests/schedulers/test_scheduler_ddpm_parallel.py -q: 40 passedmake modified_only_fixupmake fix-copiesSelf-review
Before submitting