Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix DDIM on Windows not using int64 for timesteps #819

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

hafriedlander
Copy link
Contributor

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 13, 2022

The documentation is not available anymore as the PR was closed or merged.

@patrickvonplaten
Copy link
Contributor

Hey @hafriedlander,

Thanks a lot for the issue. In general, we tend to check the library much more with Linux or Mac, but we want to be better at providing full support for Windows as well.

Just to understand better, do you know why we need to cast the int values to 64.

In general this change seems fine to me though - what do you think @anton-l ?

@hafriedlander
Copy link
Contributor Author

Windows numpy uses int32 values as the standard integer, regardless of platform. So timesteps becomes an array of int32.

We pass in individual timestep member into add_noise, and index alphas_cumprod with it. But you can't use an int32 as a tensor index in PyTorch, it complains.

Linux defaults to int64 so you don't see it there. (This would probably also break on i386 Linux, just that there aren't many of those around any more :) )

@patrickvonplaten
Copy link
Contributor

Thanks a lot for the explanation! Just wondering - isn't int32 the better dtype here? I cannot really think of a case where we would need a number higher than 2**31 for time steps ? Does this PR solve a use case that currently doesn't work?

@patrickvonplaten
Copy link
Contributor

Also cc @anton-l

@hafriedlander
Copy link
Contributor Author

This line fails with int32

sqrt_alpha_prod = self.alphas_cumprod[timesteps] ** 0.5

PyTorch requires index tensors to be Long

@anton-l
Copy link
Member

anton-l commented Oct 18, 2022

Makes sense @hafriedlander, thanks for checking!

@anton-l anton-l merged commit a3efa43 into huggingface:main Oct 18, 2022
kumquatexpress pushed a commit to harvestlabs/diffusers that referenced this pull request Oct 19, 2022
prathikr pushed a commit to prathikr/diffusers that referenced this pull request Oct 26, 2022
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants