search

flatironinstitute / mountainsort5

MountainSort spike sorting algorithm, version 5
Apache License 2.0
34 stars 8 forks source link

TemporaryDirectory context manager failing on Windows #31

Closed zm711 closed 5 months ago

zm711 commented 7 months ago

Hey Jeremy,

Just wanted to bring to the repos attention another spikeinterface issue with the TemporaryDirectory context manager failing when it tries to clean up. So far seems just to be on Windows. I haven't seen it yet, but if it happens on one of my datasets I will post here and see if I can troubleshoot it.

https://github.com/SpikeInterface/spikeinterface/issues/2489 https://github.com/SpikeInterface/spikeinterface/issues/2471

zm711 commented 7 months ago

I've now seen this on my data and discussed this in https://github.com/SpikeInterface/spikeinterface/issues/2489. The issue appears to just be the TemporaryDirectory within SpikeInterface itself and not a problem with the code here. Not sure if you want to close the issue here and open one in SI or not.

magland commented 7 months ago

Thanks @zm711 I'll leave this open here for now, in case someone else has this trouble. But will close it once the SI issue gets resolved. But let me know if anything should be done on the ms5 side.

zm711 commented 7 months ago

Honestly scheme 2 has been working very well (maybe even better than ks3 for some of my data I just have to ignore_cleanup_errors, but we can't have that on SI yet since our min version is < 3.10 still). And with the binary file saved it has been running faster too. Are you okay if we remove the context manager on spikeinterface side? I think the issue is that this just won't work with windows. Maybe locally I can play around and see if I can find an optimal windows cleanup and then we can propagate on SI and tag you for final approval?

magland commented 7 months ago

@zm711 Feel free to modify the SI wrapper as you see fit.

zm711 commented 5 months ago

We fixed this on the SI side so I'm closing this issue.