-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Add timestep_spacing to schedulers #3929
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
| self.num_inference_steps = num_inference_steps | ||
|
|
||
| # "leading" and "trailing" corresponds to annotation of Table 1. of https://arxiv.org/abs/2305.08891 | ||
| # "leading" and "trailing" corresponds to annotation of Table 2. of https://arxiv.org/abs/2305.08891 |
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.
missing linspace from ddim, will add later
| dynamic_thresholding_ratio: float = 0.995, | ||
| clip_sample_range: float = 1.0, | ||
| sample_max_value: float = 1.0, | ||
| timestep_spacing: str = "leading", |
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.
default is leading here
| if self.config.timestep_spacing == "linspace": | ||
| timesteps = np.linspace(0, self.config.num_train_timesteps - 1, num_inference_steps).round()[ | ||
| ::-1 | ||
| ].copy().astype(np.int64) |
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.
Need to be careful here as some schedulers use float and others use ints.
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.
How about performing the type-casting after all the conditions have been traversed?
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.
Good idea, I'll check it out.
| trained_betas: Optional[Union[np.ndarray, List[float]]] = None, | ||
| use_karras_sigmas: Optional[bool] = False, | ||
| prediction_type: str = "epsilon", | ||
| timestep_spacing: str = "linspace", |
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 one defaults to linspace
|
The documentation is not available anymore as the PR was closed or merged. |
| @property | ||
| def init_noise_sigma(self): | ||
| # standard deviation of the initial noise distribution | ||
| if self.config.timestep_spacing == "linspace": |
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.
I think this should be the case for trailing too.
| timestep_spacing (`str`, default `"leading"`): | ||
| The way the timesteps should be scaled. Refer to Table 2. of [Common Diffusion Noise Schedules and Sample | ||
| Steps are Flawed](https://arxiv.org/abs/2305.08891) for more information. |
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.
Note: steps_offset already existed here, I put timestep_spacing before it for consistency.
| self._timesteps += self.config.steps_offset | ||
| # "linspace", "leading", "trailing" corresponds to annotation of Table 2. of https://arxiv.org/abs/2305.08891 | ||
| if self.config.timestep_spacing == "linspace": | ||
| self._timesteps = np.linspace(0, self.config.num_train_timesteps - 1, num_inference_steps).round().astype(np.int64) |
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.
Note they are not reversed in this case
|
One problem is that import torch
from diffusers import DiffusionPipeline
from diffusers import HeunDiscreteScheduler
pipe = DiffusionPipeline.from_pretrained("runwayml/stable-diffusion-v1-5", torch_dtype=torch.float16)
pipe = pipe.to("cuda")
pipe.scheduler = HeunDiscreteScheduler.from_config(pipe.scheduler.config)
# Here we are using heun with `leading` instead of the previous default `linspace`@patrickvonplaten how do you think we should deal with this? |
Not sure the range is the way it was intended.
I thought about this too previously. Can we do the following here: We add a mechanism for values that are not loaded from the config: https://huggingface.co/runwayml/stable-diffusion-v1-5/blob/main/scheduler/scheduler_config.json and then default to the value in the |
For params not present in the original configuration. This makes it possible to switch pipeline schedulers even if they use different timestep_spacing (or any other param).
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
This test currently fails in main. When switching from DEIS to UniPC, solver_type is "logrho" (the default value from DEIS), which gets translated to "bh1" by UniPC. This is different to the default value for UniPC: "bh2". This is where the translation happens: https://github.com/huggingface/diffusers/blob/36d22d0709dc19776e3016fb3392d0f5578b0ab2/src/diffusers/schedulers/scheduling_unipc_multistep.py#L171
|
This is ready for review now. I'm not fully sure that the |
Fixes a bug when switching from UniPC from another scheduler (i.e., DEIS) that uses a different solver type. The solver is now the same as if we had instantiated the scheduler directly.
|
PR looks great to me - however could we merge this into "main" and not the |
|
Rebased in #3947. |
No description provided.