search

GWeindel / hmp

Repository for the hmp python package
BSD 3-Clause "New" or "Revised" License
35 stars 8 forks source link

Single core support for hsmm.backward_estimation() #35

Closed JoKra1 closed 1 year ago

JoKra1 commented 1 year ago

Hey Gabriel,

Two reasons for this issue:

Reason 1: In case we want to parallelize over subjects in the adjusted LOOCV as discussed in #34 we need support for:

init = hsmm.models.hsmm(hsmm_data, sf=eeg_data.sfreq, bump_width=50, cpus=1)
fit= init.backward_estimation(max_starting_points=1)

which would be called in the proposed __loocv_backwards() function but currently results in a ValueError: For loop not yet written use cpus >1 (which I think should actually be ValueError: For loop not yet written use cpus < 2 :-)). I implemented this in caa5930 if you want to take a look.

Reason 2:

Based on the implementation in caa5930, there appears to be no benefit of using multiple cores for this one on the data from tutorial 3. Calling:

init = hsmm.models.hsmm(hsmm_data, sf=eeg_data.sfreq, bump_width=50, cpus=4)
fit= init.backward_estimation(max_starting_points=1)

reports a wall time of 14.2 seconds on my machine while calling

init = hsmm.models.hsmm(hsmm_data, sf=eeg_data.sfreq, bump_width=50, cpus=1)
fit= init.backward_estimation(max_starting_points=1)

reports a wall time of 13.9 seconds. So the cost of spawning more processes appears to actually be higher than the gain of splitting the work. This will probably depend on a lot of factors (number of trials probably?) but I think it is another good reason why this should be added.

I did not open a PR though because there is this mp parameter in the fit_single() function that I do not fully understand. I assumed that I would have to set that one to False in my implementation but it only worked with it being set to True. I was not sure whether this is worth another issue, but maybe you can explain this parameter to me. :-)

GWeindel commented 1 year ago

Tutorial 3 is only on 4 participants so the gain in parallelization is very weak but as soon as the fit takes longer with more participants the cost of spawning processes will be overruled by the benefit of having several processes for each solution. You can try with the data by Anderson and Borst referenced in Tutorial 1.

Regarding the mp parameter it is a poor-man fix, I'll get rid of it in the next version for now it just transposes the magnitudes matrix.

More generally I would advise to keep any PR before version 0.1.0 that I will push beginning of next week probably.