fooof icon indicating copy to clipboard operation
fooof copied to clipboard

Performance "regression" in 2.0.0rc2

Open toni-neurosc opened this issue 8 months ago • 1 comments

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 the curve_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

toni-neurosc avatar Jun 17 '24 10:06 toni-neurosc