Closed mspronesti closed 1 year ago
Re GSL do you know how complex it would be to re-implement this api? Maybe there is some pytorch bits that could be leveraged here somehow? My understanding is that the added crate are bindings for some library that has to be installed on the user systems so that may be a bit of a restriction.
Yesterday I asked myself that same question and, before coding anything, I looked up on GitHub. I found this implementation which looks like the same API. The README says it is based on gsl, but, to me, it seems that It doesn't depend on it at all. Perhaps they meant they tried to re-implement gsl in rust without binding it. If it is the case we might "copy and adapt" some of this code.
How about using quadrature ? It (almost) only contains the quadrature::integrate
function we are interested in. We might even simply copy the two files under this directory (giving credit to the author) if you prefer, but I'm not sure a directory of utils is really worth it. To me it's easier to simply add this dependency :)
The above changes make the scheduler work as expected and all the checks pass. E.g.
Maybe we could have this dependency optional behind a feature gate and turned off by default (it sounds a bit sad to require users to have the library installed even if it's a common one).
Do you mean something like this ?
Cargo.toml
[dependencies] # ... GSL = { version = "6.0", optional = true }
[features]
lms-discrete = [ "dep:GSL" ]
> src/scheduers/mod.rs
```rs
#[cfg(feature = "lms-discrete")]
pub mod lms_discrete;
which implies adding an extra --features lms-discrete
after cargo run
to use the scheduler.
If it is the case, yep, seems a valuable option to me, even if I recommend considering the quadrature
lib I mentioned in my previous comment, which is multiplatform and seems to be working just as good as gls for the integrand this scheduler has to deal with. I'm 100% it is no way close to GLS in general, but for this specific use case seems just as fast and as precise.
Let me know what you prefer and I will adapt the scheduler :)
Yeah using this quadrature
one sounds probably better if it's pure rust / without dependency. That said, the crate hasn't been touched in the last ~5y so I would prefer the files being copied over if it's only two of them, and indeed we should add to the top of the files the original credit as well as a pointer to the original repo.
Sure :) Any suggestion on where to place them (e.g. a separate file under src/schedulers) ? One of the two is just a vector of constants, so we might even consider putting everything in one file.
Everything in one file sounds good indeed, I would say in src/schedulers
for now and if somehow it becomes helpful to other parts we could move it to a src/utils
at this point. Sounds reasonable?
Sure :) I will append a new commit to this PR in a sec. As far as I know, this scheduler is the only part of diffusers (currently, at least) that requires integration.
Done!
I recommend trying it first, possibly comparing the performances to GSL. Based on my experiments, the produced images match entirely, but the benchmarks might not be that reliable: I could only run some experiments on my CPU as I don't have enough memory on my NVidia GPU. I think that if you spot any significant difference in terms of speed, perhaps resorting to your original idea of the feature
gate might be better in the end :)
Merged, looks great thanks!
Hi @LaurentMazare , this PR aims at integrating the LMS Discrete Scheduler into this repository solving the third task mentioned in #23. This implementation ports all the features implemented in the HF python version.
Some observations:
scipy.integrate.quad
which binds theQUADPACK
routine written in Fortran and this scheduler is the only part in which HF diffusers needs scipy as a dependency.step
method has an additional parameterorder
. This might be problematic if we eventually opt to introduce aScheduler
trait (#36). A possible workaround might be to, instead, have an attributeorder
defaulted to 4 and a setter. For the moment, I preferred to stick with the same prototype of the Python version, but if you feel the other solution is better (or if you have something better in mind), let me know and I will append a commit to this PR.derivatives
.Vec
implementsremove
, which does the job but retrieves the removed element, which makes the clippy lint complain about the returned value being unused. I find this quite uglytherefore I opted to use
drain
with a single-element range instead. Let me know if you like it or if you prefer the other option or if you have any better idea.EDIT: I guess all this failures are due to the fact that GSL needs to be installed before running the actions, as instructed here.