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

feat: add repaint #974

Merged
merged 15 commits into from
Nov 3, 2022
Merged

feat: add repaint #974

merged 15 commits into from
Nov 3, 2022

Conversation

Revist
Copy link
Contributor

@Revist Revist commented Oct 25, 2022

Added repaint pipeline based on https://arxiv.org/pdf/2201.09865.pdf @anton-l

In tests i was not sure where you take numbers to compare against from so I run the pipeline and pasted what it produces for me:
[0.14537135, 0.10728511, 0.08822048, 0.15828621, 0.11806837, 0.11007798, 0.15231332, 0.1214554, 0.15475643]

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 25, 2022

The documentation is not available anymore as the PR was closed or merged.

@anton-l anton-l self-requested a review October 25, 2022 10:43
Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

Hi @Revist! Excited to see a working RePaint port, thank you so much for working on it!

Overall the implementation is super nice, just left a couple of suggestions to bring it closer to the Stable Diffusion inpainting API (which got added after my original repaint PR).

Now I wonder if it's possible to move the noising & masking logic from RePaintScheduler to RePaintPipeline, so that it can be used with other schedulers like PNDM or K-LMS? By using just step() and add_noise(), similar to the unsupervised inpainting pipeline for Stable Diffusion: https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion_inpaint_legacy.py#L46
What do you think, would that be possible to do here?

tests/pipelines/repaint/test_repaint.py Outdated Show resolved Hide resolved
src/diffusers/pipelines/repaint/pipeline_repaint.py Outdated Show resolved Hide resolved
src/diffusers/pipelines/repaint/pipeline_repaint.py Outdated Show resolved Hide resolved
src/diffusers/schedulers/scheduling_repaint.py Outdated Show resolved Hide resolved
src/diffusers/schedulers/scheduling_repaint.py Outdated Show resolved Hide resolved
Revist and others added 5 commits October 27, 2022 12:41
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
@Revist
Copy link
Contributor Author

Revist commented Oct 28, 2022

Hi @anton-l !

Was doing some reading as i was not that familiar with these, but it seems that although theoretically that should be possible (after all RePaint just repeats some steps) in practice you have some problems like here:
https://github.com/huggingface/diffusers/blob/main/src/diffusers/schedulers/scheduling_pndm.py#L209
where the step depends on some counter and not only on timestep itself. To include PNDM first thing would be to remove such dependency.
Then one could build repaint timesteps and ask with step() and add_noise() to go forward and backward, where how to go forward would be recognized by timestep value and scheduler name.

@Revist
Copy link
Contributor Author

Revist commented Oct 28, 2022

Reading further in to make a step with PNDM you do not need a single model_output at single time but at t, t+delta/2 and t+delta (from different inputs in fact so we have 4 passes) so that changes logic here:
https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/pndm/pipeline_pndm.py#L85
where you would like to give a single model_output to step, you would have to pass model (unet) and image (x_t) instead.
You skipped this problem by creating virtual steps in this scheduler which are not really steps, then using this counter. But i would say the problems arise from this (and makes code complex imo).
In other words it ought to be true that if self.timesteps contains some step then this is really a step in time and not some placeholder for making calculations.

@Revist
Copy link
Contributor Author

Revist commented Nov 2, 2022

@anton-l I am not sure if you agree with the above comments. If so I could take a look at how to change the code to the PNDM (and possibly other) schedulers however that seems like significant changes across code.

@anton-l
Copy link
Member

anton-l commented Nov 2, 2022

@Revist I've tried refactoring it a bit and completely share your concerns now, let's leave the scheduler as is for now!

Pinging @patrickvonplaten @patil-suraj for a second review, as this is a new pipeline and scheduler :)

"https://huggingface.co/datasets/hf-internal-testing/diffusers-images/resolve/main/"
"repaint/celeba_hq_256_result.png"
)
expected_image = np.array(expected_image, dtype=np.float32) / 255.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok for now, but let's make sure to pass to numpy image tests in the very near future

Copy link
Member

Choose a reason for hiding this comment

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

Will fetch the numpy images for all integration tests in a bulk PR 👍

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.

Looks good to me ! Before merging let's please make sure to add it to the docs as well and maybe reach out to the original author here?

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
@anton-l
Copy link
Member

anton-l commented Nov 2, 2022

Added the docs for the pipeline: https://moon-ci-docs.huggingface.co/docs/diffusers/pr_974/en/api/pipelines/repaint
And the scheduler: https://moon-ci-docs.huggingface.co/docs/diffusers/pr_974/en/api/schedulers#diffusers.RePaintScheduler

Gently pinging the RePaint author @andreas128 for an optional review and any comments, in case out email thread gets lost 🤗

@anton-l anton-l merged commit d38c804 into huggingface:main Nov 3, 2022
@anton-l
Copy link
Member

anton-l commented Nov 3, 2022

@Revist the pipeline will be included in today's diffusers==0.7.0 release! 🥳

If you're interested, maybe we could work together on a Spaces demo similar to SD-inpainting https://huggingface.co/spaces/runwayml/stable-diffusion-inpainting to promote the pipeline further, let me know what you think!
Also would be interesting to try integrating OpenAI's improved-diffusion models to completely reproduce the RePaint paper 🔥

@Revist
Copy link
Contributor Author

Revist commented Nov 4, 2022

@anton-l Sounds interesting. What models would you like to integrate from improved diffusion? I was thinking of taking a look into #894 as this may be of use to our group in the near future. Maybe we could speak on some other communicator?

@Revist Revist deleted the fja_add_repaint branch November 11, 2022 13:09
@anton-l
Copy link
Member

anton-l commented Nov 17, 2022

@Revist ping me ( @anton-l ) on the HF discord: https://discord.gg/G7tWnz98XR
We have a diffusers core-contributors channel there, where we can discuss everything further :)

yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* feat: add repaint

* fix: fix quality check with `make fix-copies`

* fix: remove old unnecessary arg

* chore: change default to DDPM (looks better in experiments)

* ".to(device)" changed to "device="

Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>

* make generator device-specific

Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>

* make generator device-specific and change shape

Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>

* fix: add preprocessing for image and mask

Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>

* fix: update test

Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>

* Update src/diffusers/pipelines/repaint/pipeline_repaint.py

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

* Add docs and examples

* Fix toctree

Co-authored-by: fja <fja@zurich.ibm.com>
Co-authored-by: Anton Lozhkov <aglozhkov@gmail.com>
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
Co-authored-by: Anton Lozhkov <anton@huggingface.co>
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.

4 participants