Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Q] Possibly unused self.final_alpha_cumprod #9395

Open
fdtomasi opened this issue Sep 9, 2024 · 7 comments
Open

[Q] Possibly unused self.final_alpha_cumprod #9395

fdtomasi opened this issue Sep 9, 2024 · 7 comments
Labels
stale Issues that haven't received updates

Comments

@fdtomasi
Copy link

fdtomasi commented Sep 9, 2024

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

# 1. get previous step value
prev_step_index = self.step_index + 1
if prev_step_index < len(self.timesteps):
prev_timestep = self.timesteps[prev_step_index]
else:
prev_timestep = timestep
# 2. compute alphas, betas
alpha_prod_t = self.alphas_cumprod[timestep]
alpha_prod_t_prev = self.alphas_cumprod[prev_timestep] if prev_timestep >= 0 else self.final_alpha_cumprod

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 fdtomasi changed the title [Q] Posibly unused self.final_alpha_cumprod [Q] Possibly unused self.final_alpha_cumprod Sep 9, 2024
Copy link

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 github-actions bot added the stale Issues that haven't received updates label Oct 12, 2024
@fdtomasi
Copy link
Author

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

@github-actions github-actions bot removed the stale Issues that haven't received updates label Oct 14, 2024
@yiyixuxu
Copy link
Collaborator

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?

@fdtomasi
Copy link
Author

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!

@yiyixuxu
Copy link
Collaborator

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

@fdtomasi
Copy link
Author

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 :)

Copy link

github-actions bot commented Nov 9, 2024

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 github-actions bot added the stale Issues that haven't received updates label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues that haven't received updates
Projects
None yet
Development

No branches or pull requests

2 participants