diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

[Q] Possibly unused `self.final_alpha_cumprod`

Open fdtomasi opened this issue 1 year ago • 6 comments

Hello team, quick question to make sure I understand the behavior of the step function in LCM Scheduler.

https://github.com/huggingface/diffusers/blob/a7361dccdc581147620bbd74a6d295cd92daf616/src/diffusers/schedulers/scheduling_lcm.py#L534-L543

Here, it seems that the condition prev_timestep >= 0 is always True, because timestep and self.timesteps[prev_step_index] cannot be negative. This would mean that self.final_alpha_cumprod is never used. Is there a way in which prev_timestep can be negative?

fdtomasi avatar Sep 09 '24 17:09 fdtomasi

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.

github-actions[bot] avatar Oct 12 '24 15:10 github-actions[bot]

Commenting to remove "stale" label. I think this should be addressed as it may highlight a mismatch on what timestep means in this case.

fdtomasi avatar Oct 14 '24 10:10 fdtomasi

thanks for the issue! and sorry that I missed this earlier you're right - would you be willing to open a PR to help us fix this?

yiyixuxu avatar Oct 14 '24 22:10 yiyixuxu

Thank you, no worries only commented to remove the automatic labeling.

I am not sure on the issue though. Is the solution replacing prev_timestep >= 0 with prev_timestep > 0 (so, overriding timesteps[0])? As prev_timestep is always one step higher than timestep (because they start at maximum and decrease to 0), probably what was in mind was to put alpha_prod_t = self.alphas_cumprod[prev_timestep] if prev_timestep >= 0 else self.final_alpha_cumprod, not alpha_prod_t_prev. It is not clear to me if the logic is wrong or it is just a bug, so I wanted more elucidations if possible. Thanks!

fdtomasi avatar Oct 15 '24 08:10 fdtomasi

I think the condition is just to make sure we are at the final step, so maybe prev_step_index == len(self.timesteps)? feel free to test it out

yiyixuxu avatar Oct 16 '24 01:10 yiyixuxu

As far as I understand at the final step, step_index should be 0 and prev_step_index = 1? Because timesteps should start from len(self.timesteps) and decrease. But this is why I wanted to raise this point, less to fix a bug (if any) but to clear the logic on the sampler :)

fdtomasi avatar Oct 16 '24 09:10 fdtomasi

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.

github-actions[bot] avatar Nov 09 '24 15:11 github-actions[bot]