mne-tools / mne-features

MNE-Features software for extracting features from multivariate time series
https://mne-tools.github.io/mne-features/
BSD 3-Clause "New" or "Revised" License
139 stars 32 forks source link

Behavior of edge parameter in compute_spect_edge_freq not consistant with documentation #51

Closed guiweber closed 5 years ago

guiweber commented 5 years ago

The description for the optional edge parameter of univariate.compute_spect_edge_freq is as follow:

If not None, the values of edge are assumed to be positive and will be normalized to values between 0 and 1. Each entry of edge corresponds to a percentage. The spectral edge frequency will be computed for each different value in edge. If None, edge = [0.5] is used.

There are two issues that makes the actual behavior unexpected when compared to the documentation:

  1. The sentence If None, edge = [0.5] is used implies that passing edge = [0.5] to the function should give the same results as calling the function without the parameter. It is however not the case. To get the same results, edge = [50] must be passed to the function.

  2. If edge is not None, its values are not actually "normalized" as the documentation says, but are simply divided by 100 as shown in the code below

if edge is None:
        _edge = [0.5]
    else:
        _edge = [e / 100. for e in edge]

I suggest fixing either by removing the explanation about normalization, removing the division and let the user enter a value between 0 and 1, or by making the current behavior clearer in the documentation. A better explanation of the current behavior could be as follow:

If not None, the entries of edge should be positive values between 0 and 100, each corresponding to a percentage. The spectral edge frequency will be computed for each value in edge. If None, edge = [50] is used.

jbschiratti commented 5 years ago

Thank you @guiweber for your feedback. Clearly, the doc of univariate.compute_spect_edge_freq can be improved (as per your suggestions). I'll fix that.

jbschiratti commented 5 years ago

@guiweber #52 should fix it. If it does not completely address your concerns, do not hesitate to comment on the PR.

guiweber commented 5 years ago

Thanks!