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

fix: dpm_solver shouldn't be required to "start" on step 0 #193

Closed

Conversation

brycedrennan
Copy link
Contributor

@brycedrennan brycedrennan commented Jan 19, 2024

Example: img2img pipelines often start somewhere in the middle based on how much of the source image they want to keep.

It wouldn't surprise me that you guys know a better way to allow starting in the "middle" of a diffusion process, but this did get my test suite passing.

Example: img2img pipelines often start somewhere in the middle based on how much of the source image they want to keep
@catwell
Copy link
Member

catwell commented Jan 19, 2024

Ah thanks, I missed that in my previous PR indeed.

We need a test for this in refiners, ideally one that runs on CPU.

Isn't doing it like this (with a scheduler attribute, like it was the case earlier) also a problem if you run several loops though?

@catwell
Copy link
Member

catwell commented Jan 19, 2024

Here is an alternative idea that does not have the problem of keeping a hidden state:

  • we add a set_first_step on LatentDiffusionModel (or an extra parameter to set_num_inference_steps);
  • we take it into account for init_latents, steps, and pass it to schedulers as an argument.

@brycedrennan
Copy link
Contributor Author

Isn't doing it like this (with a scheduler attribute, like it was the case earlier) also a problem if you run several loops though?

You mean multiple image generations right? I make a new scheduler instance for each generation. In my mind, the schedulers already store state about the image generation via "num_inference_steps" (which can vary from generation to generation).

@catwell
Copy link
Member

catwell commented Jan 19, 2024

@brycedrennan Check out #201 and tell me what you think. I made the first step a 1st class citizen in schedulers and SD, like the number of steps. This way you can just initialize SD and call set_inference_steps when you change the params.

@catwell catwell closed this Jan 19, 2024
@catwell catwell reopened this Jan 19, 2024
@brycedrennan
Copy link
Contributor Author

Looks good!

@brycedrennan brycedrennan deleted the bd/dpm-start-step branch January 19, 2024 16:37
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.

2 participants