Skip to content

Conversation

@vishnu-anirudh
Copy link
Contributor

@vishnu-anirudh vishnu-anirudh commented Sep 24, 2022

Hello

I was going through the code repo and I observed how the self.betas is being assigned. I don't know if what I observed is correct, so I wanted to raise this PR to clarify this.

Without the changes made in this PR, it seems the following code is not useful

if trained_betas is not None:
    self.betas = np.asarray(trained_betas)

This is because self.betas is assigned again as part of the if-statement below. And if beta_schedule doesn't exist then an error is raised. So it doesn't matter if self.betas is assigned to trained_betas.

As a note, certain schedulers such as FlaxDDIMScheduler, DDPMScheduler and FlaxDDPMScheduler seem to have the right implementation.

If you think this is not right, I am happy to close this PR.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 24, 2022

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

@pcuenca pcuenca changed the title correcting the beta value assignment trained_betas ignored in some schedulers Sep 24, 2022
@pcuenca
Copy link
Member

pcuenca commented Sep 24, 2022

This does look like a bug to me, when trained_betas is used instead of one of the beta schedules. I updated the title, I hope that's ok with you @vishnu-anirudh.

Requesting review from others in case I'm missing something.

@pcuenca pcuenca requested review from anton-l and kashif September 24, 2022 19:21
@kashif
Copy link
Contributor

kashif commented Sep 24, 2022

@vishnu-anirudh nice catch!

@kashif
Copy link
Contributor

kashif commented Sep 25, 2022

Although the issue still remains if trained betas and schedule type is passed no?

@vishnu-anirudh
Copy link
Contributor Author

Although the issue still remains if trained betas and schedule type is passed no?

From the code, it seems we choose trained_betas by default. Otherwise, we initialise self.betas based on beta_schedule.

@patrickvonplaten
Copy link
Contributor

I think we are intertwining two things here:

This PR looks good to me :-)

@vishnu-anirudh
Copy link
Contributor Author

Hello (cc @patrickvonplaten)
Can this be merged if there are no more changes (as I don't have the rights to do it).

Thanks in advance.

@patrickvonplaten patrickvonplaten merged commit 3dacbb9 into huggingface:main Sep 29, 2022
@vishnu-anirudh vishnu-anirudh deleted the correcting_betas_assignment branch September 29, 2022 17:33
prathikr pushed a commit to prathikr/diffusers that referenced this pull request Oct 26, 2022
* correcting the beta value assignment

* updating DDIM and LMSDiscreteFlax schedulers

* bringing back the changes that were lost as part of main branch merge
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* correcting the beta value assignment

* updating DDIM and LMSDiscreteFlax schedulers

* bringing back the changes that were lost as part of main branch merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants