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

UniPC Sampler support #12

Merged
merged 3 commits into from
Feb 16, 2023
Merged

UniPC Sampler support #12

merged 3 commits into from
Feb 16, 2023

Conversation

Schorny
Copy link
Contributor

@Schorny Schorny commented Feb 12, 2023

this is more a discussion than a finalized PR

  1. UniPC is the first sampler that is implemented locally in sdkit, not just imported. so we need a good location and naming guideline.

  2. UniPC is also highly customizable with a few settings. how to expose them to the api?

  3. we need good sample settings. i did a testrun with random settings and figured that
    lower_order_final should always be True
    thresholding should always be False

order=3 seems to be more vibrant sometimes
vary_coeff seems to create more detailed backgrounds

I created my sample settings on a quick look through my generated sample images. there is definitely way more to be done here.

@Schorny
Copy link
Contributor Author

Schorny commented Feb 12, 2023

useful links:

automatic1111 feature request:
AUTOMATIC1111/stable-diffusion-webui#7705
automatic1111 implementation (WIP):
AUTOMATIC1111/stable-diffusion-webui#7710
official repo:
/~https://github.com/wl-zhao/UniPC

@@ -34,6 +34,7 @@ def generate_images(
hypernetwork_strength: float = 0,

callback=None,
sampler_params={}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a trailing comma (,)

'lower_order_final': True,
'thresholding': False,
},
'unipc_snr': { #same as suggested version
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need duplicate entries? Maybe we can keep a single entry here (the snr one), and update the documentation to recommend one?

@cmdr2
Copy link
Contributor

cmdr2 commented Feb 13, 2023

Hi @Schorny , thanks for this PR! Nice work!

  1. Instead of custom, I'd suggest this folder structure:
sampler/
    sampler_main.py
    ...
    unipc_samplers/
        __init__.py # contains the code currently in unipc_samplers.py (with an 's')
        unipc_sampler.py # with the actual implementation
  1. Regarding parameters, your sampler_params approach seems like the correct one. It allows us to keep sampler-related params in a single container, instead of flattening it into the function call.

Please ensure (if you haven't already) that good defaults are used, if no sampler params are passed.

Once this PR is merged, we can look at the Easy Diffusion UI separately.

Thanks again!

@cmdr2
Copy link
Contributor

cmdr2 commented Feb 15, 2023

Hi @Schorny Thanks for this. IMO, the PR is only pending the folder structure change. No hurry or pressure, just trying to avoid any miscommunication.

@cmdr2 cmdr2 changed the base branch from main to unipc February 16, 2023 16:04
@cmdr2 cmdr2 merged commit f5cc38a into easydiffusion:unipc Feb 16, 2023
@cmdr2
Copy link
Contributor

cmdr2 commented Feb 16, 2023

Merged this to a branch, will test it out, thanks!

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.

2 participants