TomasLenc / acf_tools

Utilities for working with autocorrelation to capture periodicity in a signal.
GNU General Public License v3.0
2 stars 0 forks source link

'f0_to_ignore' parameter name is confusing #12

Closed TomasLenc closed 9 months ago

TomasLenc commented 9 months ago

The name of the '0_to_ignore parameter is not really self-explanatory.

The reason is that it can be, in principle, used for two things in the code:

  1. When fitting the 1/f component using FOOOF, response peaks need to be first removed from the magnitude spectrum. This is the original use of this parameter, and hence it's name.
  2. When doing the "step 3" of noise correction, we want to zero-out noise bins in the FFT and only keep response frequencies. This is controlled by the only_use_f0_harmonics parameter. However, since we already had the f0_to_ignore parameter with the response frequencies, I ended up re-using that information rather than asking the user to pass the frequencies to keep in a separate argument.

Now, when another method is used instead of FOOOF, e.g. IRASA, there is no need to "ignore" the peaks at response frequencies, since the method takes care of them implicitly without knowing where they are. Still, to correctly apply step 3 of noise correction, we currently take response frequencies from the f0_to_ignore, even though they are actually not being ignored.

I suggest to rename this parameter as response_f0.

Note: Keep in mind that this would be a breaking change.

cedlenoir commented 9 months ago

I totally agree, at the very beginning I struggled to identify clearly those tow parameters, so go!