Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 13, 2024
Merged

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

merged 2 commits into from
Feb 13, 2024

Conversation

yiyixuxu
Copy link
Collaborator

@yiyixuxu yiyixuxu commented Feb 12, 2024 •
edited
Loading

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
yiyixuxu commented Feb 12, 2024
View reviewed changes
src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
@@ -320,7 +320,7 @@ def set_timesteps(self, num_inference_steps: int, device: Union[str, torch.devic

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`."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes LGTM - thanks!

yiyixuxu
yiyixuxu commented Feb 12, 2024
View reviewed changes
src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
@@ -233,7 +233,7 @@ def get_order_list(self, num_inference_steps: int) -> List[int]:
orders = [1, 2, 3] * (steps // 3) + [1, 2]
elif order == 2:
if steps % 2 == 0:
orders = [1, 2] * (steps // 2)
orders = [1, 2] * (steps // 2 - 1) + [1, 1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@HuggingFaceDocBuilderDev
Copy link

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
patrickvonplaten approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

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
Copy link
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
yiyixuxu commented Feb 12, 2024
View reviewed changes
src/diffusers/schedulers/scheduling_dpmsolver_singlestep.py
@@ -151,7 +151,7 @@ def __init__(
sample_max_value: float = 1.0,
algorithm_type: str = "dpmsolver++",
solver_type: str = "midpoint",
lower_order_final: bool = True,
lower_order_final: bool = False,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 yiyixuxu merged commit 9ea62d1 into main Feb 13, 2024
@yiyixuxu yiyixuxu deleted the fix-dpm-single branch February 13, 2024 08:10
yiyixuxu added a commit that referenced this pull request Feb 13, 2024
@yiyixuxu
[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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

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
@yiyixuxu @HuggingFaceDocBuilderDev @patrickvonplaten