Repository navigation
fix(schedulers): skip noise at terminal DDPM step when no predecessor exists - #15142
Open
mirzasnowy wants to merge 1 commit into
Open
mirzasnowy wants to merge 1 commit into
mirzasnowy wants to merge 1 commit into
Conversation
… exists With timestep_spacing='trailing', the final selected timestep is nonzero, so DDPMScheduler.step() and DDPMParallelScheduler.step() added fresh noise at the terminal transition even though the mean is already the predicted clean sample. Gate noise addition on predecessor existence (prev_t >= 0) instead of on t > 0, matching how the same methods already handle the terminal case for the alpha coefficients. Fixes huggingface#15139
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15139.
Bug
With
variance_type="fixed_large"andtimestep_spacing="trailing",DDPMScheduler.step()adds noise to the final clean-data prediction. For 1000 training timesteps and 50 inference steps, the last selected timestep is 19 andprevious_timestep(19)returns -1, butstep()still enters its noise branch because it checkst > 0. The mean at this boundary is alreadypred_original_sample(alpha_prod_t_prev = 1), so the fresh random term corrupts the returned clean prediction — and changing only the random seed changes the returned clean endpoint. The same guard exists inDDPMParallelScheduler.step().Fix
In both
step()methods, the noise branch guard changes fromt > 0toprev_t >= 0.prev_tis already computed at the top of each method and already used the same way for the alpha coefficients (alpha_prod_t_prev = ... if prev_t >= 0 else self.one), so the new condition is consistent with the method's own terminal handling. Behavior changes only at terminal steps with t > 0 (trailing spacing or custom schedules ending nonzero); all other steps are bit-identical. The parallel scheduler'sstep()is not a# Copied fromblock (verified against the file's copied-block list), so nomake fix-copiessync is owed; both guards were changed identically regardless.Tests
fixed_large 19 -1deviates0.1654895395040512for both schedulers (fixed_small0.0); after fix, all four combinations 0.0.test_terminal_step_adds_no_noisein both test files: FAIL on pristine code, PASS with the fix.test_scheduler_ddpm.py+test_scheduler_ddpm_parallel.py: 77 passed.ruff checkclean with the repo's ownpyproject.toml;ruff format --checkclean on the touched files.Before submitting
self-reviewskill on the diff?Self-review notes (final round)
Rubric:
.ai/references/review-rules.md(plus code_style copied-code rule).Blocking issues
None.
Non-blocking issues
The issue itself says "This needs maintainer review before proposing a PR."
This fix follows the reporter's suggested approach: gate noise addition on predecessor existence (
prev_t >= 0) instead of ont > 0. That condition is already how the same methods handle the terminal case two lines above (alpha_prod_t_prev = ... if prev_t >= 0 else self.one), so it is the most consistent formulation available. If a maintainer prefers a different formulation (e.g. comparing against the last timestep), the regression tests in this diff still define the required behavior exactly.The guard change sits above all variance branches (fixed_small, fixed_large, fixed_small_log, learned_range), so it applies uniformly; the reporter reproduced the bug with learned/learned_range as well. The regression tests cover fixed_small/fixed_large (the issue's exact repro). A learned_range case would need a doubled-channel model output to reach the split path; it exercises the same shared guard, so it is not added.
Dead code (advisory)
None added. The change narrows when an existing branch runs; it adds no new code paths, parameters, or classes.
Verdict
READY — fix before submitting: nothing. Leave for the actual review: item 1 above (maintainer-preferred formulation).
I have reviewed every line of this diff.