-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add the SDE variant of DPM-Solver and DPM-Solver++ to DPM Single Step #4251
base: main
Are you sure you want to change the base?
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
Gently pinging the OG DPM author @LuChengTHU Do you have an idea here by any chance? :-) |
Hi all, just see this cool PR! Thank you for the effort! The single-step sde-dpm++ may be a bit tricky and I need to carefully check this PR. Please give me some more time :) |
Thanks @LuChengTHU ! :) The second-order update confuses me - in DPM Single Step it divides by the second-last timestep, while in DPM Multi Step it divides by the last timestep. Not sure if that's intentional. Single Step:
Multi Step:
Thanks for your help! |
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. |
Sorry for the delay. will check it soon |
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. |
This PR is actually an important one - @yiyixuxu can you try taking this over? |
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. |
Not stale |
Gentle ping here @yiyixuxu |
Gentle ping again @yiyixuxu |
Gentle ping @yiyixuxu |
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. |
It has been on my to-do list for a while, but I don't have time for it right now. If anyone in the community wants to take a shot, feel free! This is an important feature to add. |
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. |
Don't think this is stale |
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. |
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. |
I want to work on continuing this PR if it is OK. |
@standardAI please!! |
Following up on our conversation at #4167 (comment)
Uses the same approach as https://github.com/huggingface/diffusers/pull/3344/files
Note: This PR is not ready to merge yet, since it's not clear whether to use
sigma_s0
orsigma_s1
in the second-order update. There is a discrepancy between the implementations of single-step and multi-step for second-order.The current Single Step implementation uses
sigma_s1
for the second-order update:diffusers/src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
Line 503 in 45f6d52
While the reference PR for Multi Step uses
sigma_s0
for the second-order update: https://github.com/huggingface/diffusers/pull/3344/files#diff-517cce3913a4b16e1d17a0b945a920e400aa5553471df6cd85f71fc8f079b4b4R478@patrickvonplaten I don't know which is the correct one. Is it a bug in multi-step, or single-step, or should I use
sigma_s1
for single-step?The test doesn't pass right now due to this error, I intentionally left the code in this state.
Thanks!