ENH: Speeding up ST clustering by 30% #195
Conversation
|
...but don't merge quite yet. A) I thought of another modification that may speed it up that I want to try, and B) I want to see what people (ahem @agramfort) think about removing the max_step option from clustering. |
| if max_step == 1: | ||
| clusters = _get_clusters_st_1step(x_in, connectivity) | ||
| else: | ||
| clusters = _get_clusters_st(x_in, connectivity, max_step) |
There was a problem hiding this comment.
It seems more natural to have _get_clusters_st carrying the logic if max_step == 1 etc.
also are these lines covered by a test as I don't see any test modified.
|
I looks good (but complicated ;-)) We should have a test that compares |
|
Fixed the function choosing as suggested by @agramfort, and added nosetests as required. Also, I sped up the nosetests by a factor of ~5 by doing 100 instead of 500 permutations (why bother). |
|
I don't think the idea I had that I mentioned earlier to speed things up further will pan out, so I'm fine with merging this for now. |
|
merged by rebase after a tiny "cosmit" I checked the test coverage of cluster_level.py and I think we can do better. Just saying... |
This increases code complexity a little bit, but it makes spatio-temporal clustering 30% faster. We can decrease code complexity back to old levels if we remove the max_step option (because then we can remove the slower method _get_clusters_st), which I don't expect to be a terribly popular option anyway and mostly included for completeness before.