-
Notifications
You must be signed in to change notification settings - Fork 6.6k
[don't merge]Img2Img: timestep mismatch in scheduler with duplicated first timesteps #5746
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
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
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. |
|
|
||
| t_start = max(num_inference_steps - init_timestep, 0) | ||
| timesteps = self.scheduler.timesteps[t_start * self.scheduler.order :] | ||
| self.scheduler._step_index_init = t_start * self.scheduler.order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a very general function call (e.g. one that every scheduler has).
We could maybe call it self.scheduler.set_begin_index(...) and then make sure that every scheduler has such a function.
| return self._step_index | ||
|
|
||
| @property | ||
| def step_index_init(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a setter method here as well (https://stackoverflow.com/questions/2627002/whats-the-pythonic-way-to-use-getters-and-setters) that can be set from all pipelines if necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but can i ask why do we need both a setter method and this set_begin_index method? they are doing exactly the same thing, no?
https://github.com/huggingface/diffusers/pull/5746/files#r1463161680
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I think we had a bit of a misunderstanding here 😅 We don't need / we shouldn't add it if it doesn't have to be used. I was under the impression that we have to use a setter method. But it seems like we don't need it after all!
In this PR I try to explore the potential timesteps mismatch between scheduler and pipeline because how we determine the first step index in
scheduler._init_step_indexmethod. I also proposed a potential solution.should fix #5687
the timestep mismatch issue
our schedulers use
step_indexto count the timestep indices, however we still need to search the index for the first timestep. In_init_step_indexmethod, when there are duplicated timesteps, we will always take 2nd one. This will potentially cause wrong simgas to be used in scheduler.For example, if we have 5 steps with a bunch of duplications
[2, 2, ,2 ,2, 1]. If the denoising loop start from the very first step, we will send the timestep2to scheduler and use it to find the first sigma. The scheduler will actually start from second step because the timestep is duplicated. I think we will get a "index out of bound" error for the last step; If the denoising loop starts from the 2nd step, we will get it just right; if we start from the 3rd step, it will be able to run but there will be a mismatch.I used below code to recreate this issue
outputs:
scheduler.timesteps:
setting initial step_index from pipeline
One solution I can think of is to set the initial step_index from the pipeline. I coded it up for stable diffusion img2img and dpm-multi scheduler as a quick example. Let me know what you think @patrickvonplaten