Description
Hi, first of all, thank you very much for developing and maintaining this package. I'm one of the contributors of py_neuromodulation, a Python tool for feature extraction from neurophysiology recordings. We include FOOOF as one of the available features that we can calculate during data analysis, and recently I came across the new renamed version SpecParam. Although we don't have a strong reason to update I wanted to give the new version a try, since the numerical methods used by FOOOF are computationally expensive and difficult to run in real-time when we want to run our analysis online.
However when I replaced the FOOOF class with SpectralModel, I noticed I was getting different output values and also that it was running 2x as slow as FOOOF. I started digging and figured out this only happens with the newer release candidate 2.0.0rc2, which is the version that pip is currently installing, while with rc1 I get the exact same results as with the FOOOF package v1.1.
It seems that the reason for the change in fitted parameters and speed is that in rc2 SciPy's curve_fit
is now being called with the default xtol, ftol and gtol parameters, as opposed to the 1E-5 set in #299 . Once I add back the tolerance to curve_fit, the results and the computation time come much closer, and once the jacobian_gauss
is added back, the results are identical (the remaining missing parameter check_finite=False
makes no difference)
Is there a reason for this change? I understand that there might be a concern with the precision of the model fit, but the doubling in the computation time is a bit expensive in use-cases like ours when we would want to be able to output real-time feature calculations. Of course, one could argue that for online analysis spectral parametrization is just not a viable metric, but I was hoping that perhaps with future performance increases it would come closer to real-time.
So, to sum up:
- What is the rationale behind dropping the looser tolerance for
curve_fit
. Were the previous tolerance values too inacurate? - Could we get a parameter in the
SpectralModel
class to set thecurve_fit
tolerance value?
EDIT: one more question
- Why was the Gaussian function jacobian dropped? It reduces computation time by 10% in my system, seems reasonable to keep it as a parameter to
curve_fit