search

treder / MVPA-Light

Matlab toolbox for classification and regression of multi-dimensional data
MIT License
70 stars 35 forks source link

ENH - hack to improve speed when doing double searchlight with naive_… #13

Closed schoffelen closed 4 years ago

schoffelen commented 4 years ago

…bayes

OK, this is a bit of Spielerei really to see whether I understand the mvpa code etc. So currently no intention to get this merged, just creating a PR to put it on @treder 's radar. Feel free to ignore.

I noticed that playing with a dataset that a double searchlight becomes extremely slow due to code design issues.

specifically, using naive_bayes, and a 4 class problem, + spatial searchlight + temporal searchlight the marginal means and variances are the double for-loop (which results in a lot of redundant computations.

My hacks here lead to a substantial speed up of the computations (10-fold or so), but they might go against @treder grand philosophy of the code.

schoffelen commented 4 years ago

Also, I haven't given it a lot of thought whether my changes break the general functionality of the code. At least it does not break ft_statistics_mvpa.

treder commented 4 years ago

speedups are always a good thing! I'll review the code in the next couple days. At first glance the train/test code looks good (I'll double check with some benchmarking). Regarding the high-level functions like mv_classify, I try to keep them as model-agnostic as possible, they're already a mess, but will have a look too

schoffelen commented 4 years ago

OK, thanks. Don't get hung up on these particular changes, I realize well that it becomes hairy if the higher level functions need to handle exceptions for specific test/train functions.

One thing which you may want to consider (I haven't thought this through in detail, so it might be nonsense) is to make a minor change to the requested API for the train functions: rather than have a parameter structure as first input, one could consider merging the hyperparameters into the cf variable, and then pass this aggregated variable into both the test and train functions. If then also the cf variable is allowed to contain some precomputed testdata related stuff + some sufficiently generic bookkeeping information, it may lead to substantial performance increase (that is for the simple classifier such as naive bayes. It even could get rid of the double for-loop in mv_classify (provided neighbours is a 1x2 cell array. Yet again, this might be something that is not generic enough so I'd be happy to be convinced that generic code might be preferred above fast code.

treder commented 4 years ago

Coming back to your PR now. There's l.40 in test_naive_bayes where you skip the for loop in naive bayes which looks nice, I'll merge it in.

For the higher-level functions, as said I'd like to keep them model agnostic, however if you have ideas about improving multi-dimensional searchlight analysis in a generic way it'd be great to hear about it.

Regarding your latter point (enhancing the cf and cfg structs), I do not see a problem per se in adding some generic information which is then passed on to the train and test functions. However, to save e.g. a for loop, I reckon that every single classifier would need to be rewritten to deal with this. This isn't a problem per se either, unless the classifiers end up replicating a lot of code internally, the resultant speedup is not significant (say 5x or more), or the speedup mostly applies to edge cases (e.g. there's limited use to speed up a 3-dimensional searchlight because it's relevant to only a small number of use cases).

I've had a similar conversation (with myself) when implementing hyperparameter tuning. I wrote the generic function mv_tune_hyperparameters which can operate on any model. However for some models, a bespoke tuning function is much faster, so for these models (e.g. Logistic Regression) I have hyperparameter tuning in the body of the train function. It makes the code more ugly, but I think it's warranted since hyperparameter tuning is so common and computationally intensive.

schoffelen commented 4 years ago

I think it would be easiest for me to just delete this PR, and make a new one regarding the for-loop

treder commented 4 years ago

OK perfect.

Am Di., 14. Juli 2020 um 15:14 Uhr schrieb Jan-Mathijs Schoffelen < notifications@github.com>:

I think it would be easiest for me to just delete this PR, and make a new one regarding the for-loop

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/treder/MVPA-Light/pull/13#issuecomment-658205086, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABV54SGRKRTD2T2CMZXMHY3R3RR5XANCNFSM4OVUE7AQ .