This PR addresses issue #891. So far, I was not able to reproduce the issue using the FiffSimulator. Also investigated the influence of window size on error and gof and could not find any indication for the observed behavior. See comments for results of the mentioned investigation.

This PR closes #891.

So, I used ex_hpiFit to run a couple of experiments to investigate the influence of window size on HPI fitting quality.

Experiment

Data

• 19 phantom data sets with hpi
• sampling frequencies in Hz: 600 (2), 1000 (6), 2000 (5) ,2053 (3), 3000 (2), 5000 (1)
• hpi frequency sets in Hz: 154-166 (6), 293-321 (10), 586-642 (3)

Changes Variables

• Window Size in samples: 200, 400, 600, 800, 1000, 2000, 3000

Measures

• mean localization error in mm
• mean Goodness of Fit in mm

Statistics

• ANOVA

Results

Mean localization error error for all 19 files per window size. Mean Goodness of Fit for all 19 files per window size.

ANOVA

• H0: mean error is different between window sizes: p = 0.0019960434118255765
• H0: mean gof is different between window sizes: p = 0.011718108853769449

Summary

• would not take too much from the statistics, but I think it clearly indicates that there is little influence of increasing window size on mean localization error. Further, there is an indication that increasing window size increases the GoF, which also makes sense since noise gets averaged out. I guess this dependency on window size is more severe in noisy environments.
• Definitely does not go along with our observation of really bad fits once window size was changed in the HPI-plugin.
• Could not reproduce the issue using the fiff-simulator
• Indicates that this problem is caused within the HPI plugin in connection with the FT Buffer.

Codecov Report

Merging #892 (6e751f7) into main (bd31738) will increase coverage by 6.02%. The diff coverage is 0.70%.

@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
+ Coverage   30.20%   36.22%   +6.02%     
==========================================
  Files         452      199     -253     
  Lines       39208    11811   -27397     
==========================================
- Hits        11841     4279    -7562     
+ Misses      27367     7532   -19835     

Impacted Files	Coverage Δ
libraries/disp/viewers/helpers/scalecontrol.h	0.00% <ø> (ø)
libraries/inverse/hpiFit/hpifit.h	0.00% <ø> (-60.00%)	:arrow_down:
libraries/inverse/hpiFit/hpimodelparameters.h	0.00% <0.00%> (ø)
libraries/inverse/hpiFit/sensorset.h	0.00% <0.00%> (ø)
libraries/inverse/hpiFit/signalmodel.h	0.00% <0.00%> (ø)
libraries/rtprocessing/rthpis.cpp	1.78% <0.00%> (-0.07%)	:arrow_down:
libraries/rtprocessing/rthpis.h	0.00% <ø> (ø)
libraries/utils/file.cpp	0.00% <ø> (ø)
libraries/utils/ioutils.cpp	23.68% <0.00%> (-6.32%)	:arrow_down:
libraries/utils/kmeans.cpp	0.27% <ø> (ø)
... and 295 more

This is very good. It is nice to verify with numbers this we've been going through. In the past we've tried to solve this problem in many different ways. One of the most promising ways was to try to tackle your last point with a new approach. But that will also bring more delays, etc... so: the stage of development of the whole feature is more or less clear. I would suggest to think at which point you want the functionality to be shipped, and we'll go from there. If it is, "as is", well ok.

There is a few things in the code which I'd comment on, just to try to help you improve your coding skills if you're interested, let me know. Otherwise I see no problem.

You are more than welcome to leave some comments :)

I would suggest to think at which point you want the functionality to be shipped, and we'll go from there. If it is, "as is", well ok.

I guess this might be something we could discuss tomorrow in a bit more detail.

Some new insides on that matter:

This seem to be a problem.

if(iDataIndexCounter + matData.cols() < matDataMerged.cols()) {
    matDataMerged.block(0, iDataIndexCounter, matData.rows(), matData.cols()) = matData;
    iDataIndexCounter += matData.cols();
} 

I'm not quite sure yet how it works, but this potentially leaves an empty part of matDataMerged, since it is not necessarily filled until the end.

Not sure. I remember going to that specific part of the code. I was suspicious that under specific window lengths, some data could've been padded with zeros at the beginning or at the end. I did try to investigate if this was the case and it all seemed to work. Being all eigen structs is a bit of a double edge sword. They are well tested and solid, but perhaps we're not using them correctly.
I think I discussed this problems with @LorenzE, and for sure with @gabrielbmotta. In the end it seemed to work. I can't remember the tests I made, or how deep they were. Maybe you find something that I missed.

Not sure. I remember going to that specific part of the code. I was suspicious that under specific window lengths, some data could've been padded with zeros at the beginning or at the end. I did try to investigate if this was the case and it all seemed to work. Being all eigen structs is a bit of a double edge sword. They are well tested and solid, but perhaps we're not using them correctly. I think I discussed this problems with @LorenzE, and for sure with @gabrielbmotta. In the end it seemed to work. I can't remember the tests I made, or how deep they were. Maybe you find something that I missed.

I agree with you, this does not seem to be an issue. I created a little toy example with the same merging algorithm as in the plugin and played around with a sine of 100 samples and different window sizes. It seems to work very nice, also for different iterations. The example can be found here https://github.com/RDoerfel/mne-cpp/commit/a9b3ae4d5473427d788589fa03056831

Following are some of the results. So it does not look like there are any chunks left out. The vertical line indicates the end of the vector. Also after multiple repetitions, it looks still good.

Here are configurations I tested. One Sine has 100 samples, so all the results are basically as expected. I repeated the for loop with the merging stuff for 5 times to see if on a later stage things go wrong, but everything looks good.

Buffer Size	Window Size	Result
100	50	5 iterations with first half of sine
100	100	5 iterations with full sine
100	150	2 iterations with 3 halfs of sine
100	300	1 iteration with 3 full sines

I think I did exactly the same thing as you did.... :)

-- Juan

On Mon, Apr 18, 2022 at 7:58 AM Ruben Dörfel @.***> wrote:

Not sure. I remember going to that specific part of the code. I was suspicious that under specific window lengths, some data could've been padded with zeros at the beginning or at the end. I did try to investigate if this was the case and it all seemed to work. Being all eigen structs is a bit of a double edge sword. They are well tested and solid, but perhaps we're not using them correctly. I think I discussed this problems with @LorenzE https://github.com/LorenzE, and for sure with @gabrielbmotta https://github.com/gabrielbmotta. In the end it seemed to work. I can't remember the tests I made, or how deep they were. Maybe you find something that I missed.

I agree with you, does not seem to be an issue. I created a little toy example with the same merging algorithm as in the plugin and played around with a sine of 100 samples and different window sizes. It seems to work very nice, also for different iterations. The example can be found [here]( @.*** https://github.com/RDoerfel/mne-cpp/commit/a9b3ae4d5473427d788589fa03056831

Here are some of the results. So it does not look like there are any chunks left out. (The vertical line indicates the end of the vector.) Also after multiple repetitions it looks still good. [image: Figure_1] https://user-images.githubusercontent.com/40984141/163804912-d3098597-27fc-4bb2-bfa8-0299f7f5227d.png

— Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-cpp/pull/892#issuecomment-1101343867, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACEKMIDDCXFVQ7OZKMGOPETVFVE7BANCNFSM5P773RGA . You are receiving this because you commented.Message ID: @.***>

Great! So it is verified from two independent sites :)

Here is a screenshot of what I was thinking. You can now set the window size (could discuss if we want to ditch it), and the time between fits. The time between fits is not super accurate since it is time between fits + time one fit needs. However, it is still around this time and allows to set slower fitting periods. The time between fits is limited by the window size. The time between fits is currently counted using the size of data poping from the buffer. Potantially, one could use QElapsedTimer to have accurate timing. However, this might introduce some other issues, and I'm not sure if it should be used that way.

What do you think @juangpc @gabrielbmotta? Could this improve the fitting on a Pi?

I'd leave it. Since it is in a menu inside the settings, I don't think it hurts to have it right now.