FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[DPMSolverSinglestepScheduler] correct `get_order_list` for `solver_order=2`and `lower_order_final=True` by yiyixuxu · Pull Request #6953 · huggingface/diffusers · GitHub

/ diffusers Public

[DPMSolverSinglestepScheduler] correct get_order_list for solver_order=2and lower_order_final=True - #6953

Merged
yiyixuxu merged 2 commits into
mainfrom
fix-dpm-single
Feb 13, 2024
ConversationCommits (2)ChecksFiles changed
Merged

[DPMSolverSinglestepScheduler] correct get_order_list for solver_order=2and lower_order_final=True #6953
yiyixuxu merged 2 commits into
mainfrom
fix-dpm-single

Conversation

yiyixuxu commented Feb 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

For lower_order_final=True and solver_order=2, we would expect the order for the last step to be 1, currently inDPMSolverSinglestepScheduler it does not work as expected for an even number of steps

this is the get_order_list method of DPMSolverSinglestepScheduler, (see my comments in code below)

diffusers/src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py

Line 216 in 0a1daad

def get_order_list(self, num_inference_steps: int) -> List[int]:

currently lower_order_final=True and lower_order_final=False have exactly same result

    def get_order_list(self, num_inference_steps: int) -> List[int]:
        steps = num_inference_steps
        order = self.config.solver_order
        if self.config.lower_order_final:
            if order == 3:
                ...
            elif order == 2:
                if steps % 2 == 0:
                    # YiYi notes: for solver_order = 2, if the steps are even, it seems that we are not lowering the final order even when `config.lower_final_order=True`
                    orders = [1, 2] * (steps // 2) 
                else:
                    # YiYi notes: for uneven steps however, it lowers the final order as expected
                    orders = [1, 2] * (steps // 2) + [1]
            elif order == 1:
              ...
        else:
           ...
           elif order == 2:
              # YiYi notes: this is exactly same as lower_final_order=True
              orders = [1, 2] * (steps // 2)
           ...
        return orders

another option is just to not allow use even number of steps + solver_orde=2 + lower_final_order - I think it's more complicated that way, especially now we have the `last_sigmas_type='zero' argument that has to work with final_order ==1

related PR /issue
#6949
#1866
#3413
#6477

yiyixuxu commented Feb 12, 2024
View reviewed changes
Comment thread src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
if not self.config.lower_order_final and num_inference_steps % self.config.solver_order != 0:
logger.warn(
"Changing scheduler {self.config} to have `lower_order_final` set to True to handle uneven amount of inference steps. Please make sure to always use an even number of `num_inference steps when using `lower_order_final=True`."
"Changing scheduler {self.config} to have `lower_order_final` set to True to handle uneven amount of inference steps. Please make sure to always use an even number of `num_inference steps when using `lower_order_final=False`."

yiyixuxu Feb 12, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it's a typo here but let me know if it's not @patrickvonplaten
from this PR #3413

patrickvonplaten Feb 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes LGTM - thanks!

yiyixuxu commented Feb 12, 2024
View reviewed changes
Comment thread src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
elif order == 2:
if steps % 2 == 0:
orders = [1, 2] * (steps // 2)
orders = [1, 2] * (steps // 2 - 1) + [1, 1]

yiyixuxu Feb 12, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

for order=2 and lower_order_final=True, currently it generate orders like 1,2,1,2...,1,2 so final order will be 2
changing it to have both of the last two final steps to be 1 here

patrickvonplaten Feb 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this only necessary for DPMSingleStepSolver? Not for the other ones?

HuggingFaceDocBuilderDev commented Feb 12, 2024

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

patrickvonplaten approved these changes Feb 12, 2024
View reviewed changes

patrickvonplaten left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good to me! Can we quickly make sure that other schedulers (like multi-step, unipc and/or deis) are not affected by this bug?

yiyixuxu commented Feb 12, 2024

Copy link
Copy Markdown
Collaborator Author

@patrickvonplaten

This looks good to me! Can we quickly make sure that other schedulers (like multi-step, unipc and/or deis) are not affected by this bug?

I think this is unique to singlestep

yiyixuxu commented Feb 12, 2024
View reviewed changes
Comment thread src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
algorithm_type: str = "dpmsolver++",
solver_type: str = "midpoint",
lower_order_final: bool = True,
lower_order_final: bool = False,

yiyixuxu Feb 12, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I changed the default value so that we keep the default behavior the same as before @patrickvonplaten
this way I don't have to update test

yiyixuxu merged commit 9ea62d1 into main Feb 13, 2024
yiyixuxu deleted the fix-dpm-single branch February 13, 2024 08:10
yiyixuxu added a commit that referenced this pull request Feb 13, 2024
[DPMSolverSinglestepScheduler] correct get_order_list for `solver_o…
c1f2609 
…rder=2`and `lower_order_final=True` (#6953)

* add

* change default

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
patrickvonplaten patrickvonplaten approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL