Add an option ignore_first_spike, which explicit allows to ignore or consider the first spike during ISI computation

[DrTaDa]

[DrTaDa]

@wvangeit My comments were there but stuck in "Pending" al of this time T_T

https://github.com/community/community/discussions/10369

[codecov-commenter]

Codecov Report

Merging #234 (3d5c6ae) into master (b2ee1da) will decrease coverage by 0.03%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   69.71%   69.68%   -0.04%     
==========================================
  Files          12       12              
  Lines        2288     2289       +1     
==========================================
  Hits         1595     1595              
- Misses        693      694       +1     

Impacted Files	Coverage Δ
efel/api.py	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[DrTaDa]

@wvangeit It was in case the user put a value < 0, but indeed it is not needed.

If the vector is uninitialized, retVal will be >=0 no ?

[wvangeit]

@wvangeit It was in case the user put a value < 0, but indeed it is not needed.

If the vector is uninitialized, retVal will be >=0 no ?

You mean retVal < 0? Well it might be, but if it isn't for some reason we end up with a segmentation fault, it would be better to protect against it in this function.

[DrTaDa]

I added the condition on retIgnore.size(), I am not sure if there is a better way

[AurelienJaquier]

Could this cause any unexpected behavior in other features that depend on ISI_values? I know that it would at least in burst_mean_freq. Although you can ignore this particular feature because I am going to change the code of the feature anyway in a future PR.

[DrTaDa]

@AurelienJaquier we kept the same default value, so it might but only if the user specifically request it. I implemented this setting specifically for burst related computation, and it worked for me.

[AurelienJaquier]

@DrTaDa Yes, but the user might want to set ignore_first_spike to False, and compute other features. I think that I will make burst_mean_freq dependent on the value of ignore_first_spike when this is merged (since burst_mean_freq depends on the relation between the indices of ISIs and indices of peaks, that changes when we take into account the 1st ISI). I think it would be worth to see if other features would also 'break' when ignore_first_spike is changed. Or alternatively you can say in the documentation that it is not supposed to be changed except for your special use case.

[DrTaDa]

@AurelienJaquier It doesn't bother me that other efeatures might have a different behaviour if a setting is changed. That is already the case for other settings. I would expect someone using this feature to know what he is doing. (In our pipelines, the settings are set per feature so it wouldn't cause us any issue).

[wvangeit]

@AurelienJaquier , do you mean that certain features assume that the first spike is ignored? If that's the case, I do indeed consider this a problem. Because in that case 'someone using this feature to know what he is doing' is not necessary true since they might not realize a certain features assumes this setting is not set.

[AurelienJaquier]

Wait... if you want not to skip the 1st ISI, can't you just use LibV5::all_ISI_values?

[AurelienJaquier]

@wvangeit I just checked, and only burst_mean_freq would need to be modified after this merge, the other features would behave as expected.

[DrTaDa]

Let's wait for #250 to be merged and then I will fix burst_mean_freq here.

[AurelienJaquier]

For the new features I added in the last PR to the master branch, you just have to adapt the burst_indices feature. It should be fairly easy, you just have to remove the comment there, make it depend on ignore_first_spike, and change the initialisation of first_ISI and count into something like:

if (ignore_first_spike){
  int first_ISI=1, count = 1;
} else {
  int first_ISI=0, count = 0;
}

[AurelienJaquier]

I would like to rename ignore_first_spike into ignore_first_ISI for clarity, since it only affects the 1st ISI, and features depending on it, and not features depending on the 1st spike in general. Do you agree @DrTaDa ?

[DrTaDa]

@AurelienJaquier I do !

[AurelienJaquier]

Changes made in the last commit :

• renamed ignore_first_spike into ignore_first_ISI
• made burst features consistent with burst_indices when ignore_first_ISI is 0
• allow the new burst suite (strict_burst_* features) to also ignore the first ISI
• since the first ISI is ignored by default, I had to change the test results for the new burst features