-
Notifications
You must be signed in to change notification settings - Fork 948
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
Implement euler a scheduler #187
base: main
Are you sure you want to change the base?
Conversation
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 submitting this PR @hanstjua! I left some early comments and requested some changes. Before fully reviewing the math, I tested generating an image with the CLI and was only able to generate noise. Could you please address the comments I left and verify that you are able to generate an image for the prompt "a high quality photo of a surfing dog"
as a sanity check before a second pass? Thanks again!
|
||
self.initNoiseSigma = sigmas.max()! | ||
|
||
self.timeSteps = linspace(0, Float(trainStepCount - 1), trainStepCount).reversed().map { Int(round($0)) } |
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.
Shouldn't the length of self.timeSteps
adhere to self.inferenceStepCount
instead of trainStepCount
?
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.
Yes, you are right. I'll make this change.
@@ -160,6 +162,7 @@ public struct StableDiffusionPipeline: ResourceManaging { | |||
switch config.schedulerType { | |||
case .pndmScheduler: return PNDMScheduler(stepCount: config.stepCount) | |||
case .dpmSolverMultistepScheduler: return DPMSolverMultistepScheduler(stepCount: config.stepCount) | |||
case .eulerAncestralDiscreteScheduler: return EulerAncestralDiscreteScheduler(randomSource: randomSource(from: config.rngType, seed: config.seed)) |
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.
The new scheduler does not take config.stepCount
as an argument, is this expected?
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.
Related: Could you please register this scheduler in the CLI enum as well?
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, this is not expected. Seems like I missed this. Thanks for pointing out.
I'll register it in CLI enum.
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.
Great, looking forward to it :)
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've made the changes, but still couldn't seem to generate a proper image. Currently still looking into what's the issue.
Thank you for your interest in contributing to Core ML Stable Diffusion! Please review CONTRIBUTING.md first. If you would like to proceed with making a pull request, please indicate your agreement to the terms outlined in CONTRIBUTING.md by checking the box below. If not, please go ahead and fork this repo and make your updates.
We appreciate your interest in the project!
Do not erase the below when submitting your pull request:
#########