-
Notifications
You must be signed in to change notification settings - Fork 6.5k
[Hacktoberfest]Fixing issues #5241 #5255
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
@kashif can you please review my pull request.I am having confusion in explaining latent_mean, latent_std and resolution_multiple variables.
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.
Thanks for your contribution! 🤗
Can you run make style to pass the CI checks?
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 60 is correct here, and it is the docstring value that needs to be changed? cc @kashif
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.
no lets switch it to 30, 60 seems too excessive right @dome272?
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.
We only use 30 with the custom timesteps. Otherwise 60 yields good performance.
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.
ok got it!
Can you run
make styleto pass the CI checks?
It looks like some things still aren't formatted correctly in the pipeline_wuerstchen_prior file. Have you run make style to automatically apply the style fixes?
@stevhliu no actually I haven't applied make style for formatting .Thanks for your advice!
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.
LGTM 👍🏽
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.
thanks!
I request to please merge this pr if it's alright. Since it has been reviewed and approved, thus I request to merge it.
@jgyfutub can you kindly run make style in the root of diffusers repo and push the styling fix
@kashif I apologize for that comment ,but after downloading all the packages and modules in a virtual environment using pip install -e ".[dev]" ,pip install -e ".[test]" , and then pytest tests\pipelines\wuerstchen\test_wuerstchen_prior.py but after that when I ran make test and make style , the error 'make' is not recognized as an internal or external command, operable program or batch file. was shown.Could you please suggest a better way to tackle this?
no problem let me fix it
okk thanks!!
src/diffusers/pipelines/wuerstchen/pipeline_wuerstchen_prior.py
Outdated
Show resolved
Hide resolved
@kashif as this pr is alright and styling is done,I sincerely request you to please merge it
Hi we can't merge until the failing test passes. #5289 was recently committed to fix this issue, so if you update your branch with the latest fix it should pass.
HuggingFaceDocBuilderDev
commented
Oct 6, 2023
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.
* Update pipeline_wuerstchen_prior.py * prior_num_inference_steps updated * height, width, num_inference_steps, and guidance_scale synced * parameters synced * latent_mean, latent_std, and resolution_multiple synced * prior_num_inference_steps changed * Formatted pipeline_wuerstchen_prior.py * Update src/diffusers/pipelines/wuerstchen/pipeline_wuerstchen_prior.py --------- Co-authored-by: Kashif Rasul <kashif.rasul@gmail.com>
* Update pipeline_wuerstchen_prior.py * prior_num_inference_steps updated * height, width, num_inference_steps, and guidance_scale synced * parameters synced * latent_mean, latent_std, and resolution_multiple synced * prior_num_inference_steps changed * Formatted pipeline_wuerstchen_prior.py * Update src/diffusers/pipelines/wuerstchen/pipeline_wuerstchen_prior.py --------- Co-authored-by: Kashif Rasul <kashif.rasul@gmail.com>
#5241 issue fixed.Please review it