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

Adjust eps for whitening in case of very small magnitude data #2070

Merged
merged 11 commits into from
Oct 26, 2023
Merged

Adjust eps for whitening in case of very small magnitude data #2070

merged 11 commits into from
Oct 26, 2023

Conversation

magland
Copy link
Member

@magland magland commented Oct 4, 2023

Resolves #2064

I explain what is happening in the comments

I ran the following test:

import numpy as np
import spikeinterface.extractors as se
import spikeinterface.preprocessing as spre

# toy example
recording, sorting = se.toy_example(num_channels=4, duration=10, seed=0)
recording_whitened = spre.whiten(recording)
traces = recording_whitened.get_traces(segment_index=0)
print(np.var(traces, axis=0))

recording2 = spre.scale(recording, gain=1e-8)
recording2_whitened = spre.whiten(recording2)
traces2 = recording2_whitened.get_traces(segment_index=0)
print(np.var(traces2, axis=0))

And got the expected variances close to 1

[1.0054402  1.0075228  0.9955653  0.96718967]
[0.99692386 0.99832404 0.98711884 0.9610572 ]

And without the adjustment the second array is very wrong

[1.0054402  1.0075228  0.9955653  0.96718967]
[1.1976793e-08 1.1291137e-08 1.2642105e-08 1.6557108e-08]

@alejoe91 alejoe91 changed the title adjust eps for whitening in case of very small magnitude data Adjust eps for whitening in case of very small magnitude data Oct 5, 2023
@alejoe91 alejoe91 added the preprocessing Related to preprocessing module label Oct 5, 2023
alejoe91
alejoe91 previously approved these changes Oct 23, 2023
@alejoe91 alejoe91 added this to the 0.99.0 milestone Oct 25, 2023
Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

@magland we discussed internally and we would like to expose epsilon and make a few changes. I cannot push to your branch. Can you allow me to do that?

Here's my working branch: https://github.com/alejoe91/spikeinterface/tree/adjust-eps-for-whitening

@alejoe91 alejoe91 dismissed their stale review October 25, 2023 15:52

Needs another look

@magland
Copy link
Member Author

magland commented Oct 25, 2023

@alejoe91 I have invited you to my fork of spikeinterface.

@magland
Copy link
Member Author

magland commented Oct 25, 2023

@alejoe91 It seems that the implementation does not quite agree with the doc string. It says

If None, eps is estimated from the data.

But then the code says

if data.dtype.kind == "f" or eps is None:

Notice the "or"

IMO, the estimation of eps should only happen if eps is None.... AND ... the default should be None => effective default is 1e-8. That's somewhat different from what you have. If you do go the route of having default eps is None, which is what I am suggesting, then it needs to be updated throughout the codebase.

@alejoe91
Copy link
Member

@magland done! Thanks for the feedback!

@alejoe91 alejoe91 merged commit 67869c5 into SpikeInterface:main Oct 26, 2023
9 checks passed
@magland
Copy link
Member Author

magland commented Oct 26, 2023

@alejoe91 looks good to me!

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

Successfully merging this pull request may close these issues.

in whiten.py, eps is too large for some datasets
2 participants