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

Solve missing clip_sample implementation in FlaxDDIMScheduler. #7017

Merged
merged 42 commits into from
Mar 8, 2024

Conversation

hisushanta
Copy link
Contributor

Hi, In this pull request add a doc-string with two required parameters to solve this problem and also work some operations to solve this problem.

The issue looks like that: #6901

Before submitting

hisushanta and others added 30 commits October 5, 2023 06:34
…standing other developers what are doing and where it's using.
This changes suggest by maintener.

Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
Add suggested text

Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
I changed the Parameter to Args text.
proper indentation set in this file.
a little bit of change in the act_fun argument line.
similar doc-string add to have in the original diffusion repository.
Comment on lines +122 to 124
clip_sample: bool = True,
clip_sample_range: float = 1.0,
set_alpha_to_one: bool = True,
Copy link
Member

Choose a reason for hiding this comment

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

Why are these two being added here?

Copy link
Member

Choose a reason for hiding this comment

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

I thought the same, but actually clip_sample and clip_sample_range are added to the config in the PyTorch schedulers, and also in the Flax version of ddpm (only clip_sample). So I guess it's ok to do the same here.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

@pcuenca WDYT?

@sayakpaul
Copy link
Member

The commit history of this is borked. Would be nice to fix it either by force-pushing or by opening a new PR.

@pcuenca pcuenca changed the title Solve missing clip_sample implementation in FlexDDIMScheduler. Solve missing clip_sample implementation in FlaxDDIMScheduler. Mar 7, 2024
Copy link
Member

@pcuenca pcuenca 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. Could you please address @sayakpaul's comments about the commit history?

src/diffusers/schedulers/scheduling_ddim_flax.py Outdated Show resolved Hide resolved
Comment on lines +122 to 124
clip_sample: bool = True,
clip_sample_range: float = 1.0,
set_alpha_to_one: bool = True,
Copy link
Member

Choose a reason for hiding this comment

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

I thought the same, but actually clip_sample and clip_sample_range are added to the config in the PyTorch schedulers, and also in the Flax version of ddpm (only clip_sample). So I guess it's ok to do the same here.

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

@pcuenca
Copy link
Member

pcuenca commented Mar 8, 2024

The commit history is strange but the diff is ok, I think it's save to squash. Thanks, @hi-sushanta!

@pcuenca pcuenca merged commit 46fac82 into huggingface:main Mar 8, 2024
15 checks passed
@hisushanta hisushanta deleted the clip_bug branch March 9, 2024 03:08
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