Refactor quality metrics tests to use fixture

[chrishalcrow]

Simplify quality metrics tests, so that they mostly share two sorting_analyzers and wrapped all the tests in main(), which may be needed for ci multiprocessing(?).

This PR moves the sorting_analyzer_simple to conftest.py, which means it can be shared by all the quality metric test files (=> only has to be computed once). Moreover, we weren't using the PCs computed for this sorting_analyzer, so I got rid of that.

On my machine, the old tests took 3 mins 17 sec and the new ones 2 mins 5 sec. Note: the tests are basically the same.

Results from gh actions with n_jobs=2 (https://github.com/SpikeInterface/spikeinterface/actions/runs/10075290248):
mac3.9: 2:22
mac3.12: 2:17
win3.9: 2:09
win3.12: 2:10
ubu3.9: 2:24
ubu3.12: 2:32

Compared to old timing (https://github.com/SpikeInterface/spikeinterface/actions/runs/10063489408):
mac3.9: 2:39
mac3.12: 2:46
win3.9: 9:17
win3.12: 9:09
ubu3.9: 3:23
ubu3.12: 3:08

So: definitely helps the windows runs! Should keep checking to see if this test time is consistent across many runs.

[zm711]

I agree I think this is essential for certain Windows run. But it is super inconsistent which Windows. (for example I haven't needed to do if name == main yet, but others have.... So weird.

[JoeZiminski]

no way! 🤯 so it depends on the Windows version?

[zm711]

I think it is less "version" and more like version + sub-version + python version + CPU-type + IDE. Who knows what magic determines this.... But one of my co-workers has to do the if name == main, but I don't on my workstation.

[JoeZiminski]

I had responded but somehow dissapeared, "Haha lol the mysteries of multiprocessing, very interesting!"

[JoeZiminski]

Hey @chrishalcrow this is really nice, great to centralise these fixtures! and find out some interesting things about pytest and multiprocessing along the way!

[zm711]

@chrishalcrow,

Why are we dropping the PC calculation--? I need to look at what the actual test is checking, but without pcs calculated we wouldn't be able to test pc metrics.

I will say in general on my windows machine that doing the PCA is the slowest step. So maybe the scipy implementation on Windows is the thing that is so variable. So removing the PC got rid of that variability, but I wonder if we need that calculation.

[chrishalcrow]

@chrishalcrow,

Why are we dropping the PC calculation--? I need to look at what the actual test is checking, but without pcs calculated we wouldn't be able to test pc metrics.

I will say in general on my windows machine that doing the PCA is the slowest step. So maybe the scipy implementation on Windows is the thing that is so variable. So removing the PC got rid of that variability, but I wonder if we need that calculation.

We do still test them using the small_sorting_analyzer, rather than the sorting_analyzer_simple, in test_calculate_pc_metrics.

The tests are a bit hodgepodge, but the aim of this PR was just to try and get the times consistent across OSs, and simplify the structure.

[zm711]

Then my guess for time is that PCA is causing variability. If we aren't testing it here then removing it gets us the speed up (again my guess)--you could add it back in see if we slowdown and then remove it before actual merge.

[JoeZiminski]

Hey @chrishalcrow @zm711 are we almost done here? (sorry @zm711 I accidentally clicked to re-request your review when I meant to click myself). I guess we can continue the code-comment discussion, but maybe it is worth porting it to an issue.

Otherwise, it's just to see if reintroducing PC metric calculating in the sorting analyzer fixture brings up the test time, to see if that was the culprit?

[zm711]

What is the actual change here? Deleting Sam's/Alessio's testing is +/-; And removal of PCA. I think I would actually prefer to see if PCA is the culprit here rather than removal and never really understand why the tests shortened.

[JoeZiminski]

Yes good point worth summarising as the debug code changes can be reverted, to summarise (let me know @chrishalcrow if I missed anything):

1. debug lines deleted (let's revert, but +1 to keep them commented, will message above)
2. get_sorting_analyzer() factored into conftest.py to avoid reuse (+1)
3. unused pca metric calculation removed (let's see if that did caused the slow down, but even if it didn't, +1 to remove it as it's unused)

[alejoe91]

@chrishalcrow can we add this back so @samuelgarcia is happy? :)

[chrishalcrow]

Hoping this is ready if tests pass

[JoeZiminski]

Looks good @chrishalcrow! One last question from above, would you mind adding pack PC metrics into the test just to see if it accounts for the change in test times? (just out of interest?) then can remove again and good to merge I think, @zm711 may want a final pass

[zm711]

This looks good to me. I would love if you would just test adding PCA back so we can assess if that is the timing issue, but I could also do that in a test PR if it is too annoying. Otherwise this is fine by me. I would recommend changing the title of the PR so it won't be misleading in the changelog, but otherwise good by me.

[zm711]

Adding that back took Windows tests from 2-4 minutes to 16-17 minutes. So my guess is that PCA stuff is not optimized for Windows. That's really good to know because my lab hates running PCA because it takes forever and that confirms this on Windows.

[zm711]

Good by me :)