Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seeding sorters #3131

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

florian6973
Copy link
Contributor

Hi!

To better control reproducibility and study sorters' variability, I was envisioning to add some seeding parameters to the existing sorter configurations.

I've done ms5 and ks4 which are quite popular, I do not know if you would like something more standardized before merging.

Thanks!

@alejoe91 alejoe91 added the sorters Related to sorters module label Jul 3, 2024
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your contribution.

This seems like a good idea but I don't know enough about how sorters work to see if this is done correctly or whether we can test it (probably not?).

Two comments though:

https://github.com/h-mayorquin/spikeinterface/blob/6645dbba97f638703b7d544154af7915af1651e3/src/spikeinterface/core/generate.py#L19-L26

And debated to some deegree here:
#1963

@florian6973
Copy link
Contributor Author

Thanks for your feedback!

So I will change the default seed param to None and call ensure_seed first.

Regarding setting the global seed, I agree it is a bad behavior but otherwise we would need to go into the spike sorter code to change how numbers are generated. And for KS4, the spikeinterface code already uses this global seeding, so I was just replicating this technique for MS5. Do you feel it is worth to work on MS5 and KS4 to change their seed strategy please?

Regarding testing, we could do an empirical test to check the consistency, but this is a bit tricky because, for instance, MS5 can be deterministic depending on the settings (no spike sampling). Would you like me to add some basic tests like this?

@h-mayorquin
Copy link
Collaborator

Regarding setting the global seed, I agree it is a bad behavior but otherwise we would need to go into the spike sorter code to change how numbers are generated. And for KS4, the spikeinterface code already uses this global seeding, so I was just replicating this technique for MS5. Do you feel it is worth to work on MS5 and KS4 to change their seed strategy please?

I see. As I mentioned I am out of my depth here, I will let people who are more familiar with those sorters comment. Thanks for the clarification.

@zm711
Copy link
Collaborator

zm711 commented Jul 7, 2024

Actually one tiny point about seeding KS4. The idea there was that the current wrapper is meant to mimic KS4 behavior (at the time of writing), which didn't allow seeding it just set its own global seed. @samuelgarcia thought this was a bad idea, but since it was mimicking KS4 we left it. So for KS4 I think it is actually important for the default to mimic the native KS4 behavior, but I think it is fine to give the user the option to change that seeding. But this is just my 2 cents. For MS5 I would suggest @magland weigh in how he thinks seeding should be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorters Related to sorters module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants