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

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

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

yiyixuxu
Copy link
Collaborator

@yiyixuxu yiyixuxu commented Feb 12, 2024

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)

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

@@ -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!

@@ -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

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.

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

@@ -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
15 checks passed
@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
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants