-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
speedup _numba_sampen kernel #22
Comments
Hi @FirefoxMetzger, Thanks for opening the issue. This sounds great. Two questions:
Assuming that the answer to these questions is No and Yes, then please do feel free to work on a PR! Thanks, |
No additional dependencies beyond numpy. That said, I would implement this in numba for this repo since we currently don't have a pipeline to build and distribute C code. The main novelty in my version of the kernel is that I use a different set of tricks to count matching windows which is more efficient in reusing intermediate results and thus reduces the constant overhead. This improvement should also be visible when implementing in numba instead of pure C.
I just compared it to antropy and, unfortunately, it produces different results. That said, it does produce the same result as my naive (but easy to read) reference implementation, which I used to test my algorithm against: from local._lib.transforms import sample_entropy as c_sampleEn
from antropy.entropy import _numba_sampen, _app_samp_entropy
import numpy as np
from math import log
def reference_implementation(sequence, window_size, tolerance):
"""Easy to read but slow reference implementation."""
sequence = np.asarray(sequence)
size = sequence.size
numerator = 0
for idxA in range(size - (window_size + 1) + 1):
for idxB in range(size - (window_size + 1) + 1):
if idxA == idxB:
continue
winA = sequence[idxA : (idxA + (window_size + 1))]
winB = sequence[idxB : (idxB + (window_size + 1))]
if np.max(np.abs(winA - winB)) <= tolerance:
numerator += 1
denominator = 0
for idxA in range(size - window_size + 1):
for idxB in range(size - window_size + 1):
if idxA == idxB:
continue
winA = sequence[idxA : (idxA + window_size)]
winB = sequence[idxB : (idxB + window_size)]
if np.max(np.abs(winA - winB)) <= tolerance:
denominator += 1
return -log(numerator / denominator)
def mne_sampleEn(data, window):
phi = _app_samp_entropy(data, order=window, approximate=False)
return -np.log(np.divide(phi[1], phi[0]))
def antropy_sampleEn(data, window):
return _numba_sampen(data, order=window, r=(0.2 * np.std(data)))
# compile numba
data = np.random.randint(0, 50, 10).astype(float)
antropy_sampleEn(data, 2)
test_sequence = np.array([0, 1, 0, 0, 1, 0, 1, 0, 0, 1, 1, 1, 1, 0, 0, 1,], dtype=float)
test_std = np.std(test_sequence)
reference_value = reference_implementation(test_sequence, 2, .2*test_std)
c_result = c_sampleEn(test_sequence, 2, .2*test_std)
antropy_result = antropy_sampleEn(test_sequence, test_std)
mne_result = mne_sampleEn(test_sequence, 2)
np.testing.assert_almost_equal(c_result, reference_value)
np.testing.assert_allclose(c_result, antropy_result) @raphaelvallat Could I trouble you to have a look at the reference implementation (code above) and see if you spot a mistake in it? |
Got it.
I am out of office and unavailable until mid-February so this is unfortunately going to have to wait, but yes I can dive into it after this date. Thanks again, |
Nice, thanks! Just to avoid confusion, I'm talking about the code from this comment: #22 (comment) . It tries to be a 1:1 implementation of the definition so that a human can quickly find any deviations. Once we have that right, I can use fuzzy testing to compare it with my optimized (and less readable) implementation to see if it does the right thing, too. For reference: One difference that I found is that my code uses |
@raphaelvallat I think antropy's current implementation is off by one. (For reference, the algorithm works by counting all pairs of windows/templates (except the self-match) for which the Chebyshev distance between the windows is smaller than Consider a window/template size of If we now compute # all size 3 windows
array([[1., 7., 0.],
[7., 0., 8.],
[0., 8., 8.],
[8., 8., 0.],
[8., 0., 9.],
[0., 9., 2.],
[9., 2., 8.],
[2., 8., 8.]]) No two rows are identical, and hence # all size 2 windows
array([[1., 7.],
[7., 0.],
[0., 8.],
[8., 8.],
[8., 0.],
[0., 9.],
[9., 2.],
[2., 8.],
[8., 8.]]) We find a match for the pair I've tried this for different sequences, and whenever there is a mismatch between my implementation and antropy's it is because of matches with the last Looking into antropy's wrapper around MNE-features, I noticed that it explicitly excludes the last window before computing (note that Lines 384 to 389 in dc84ed1
We can use MNE-feature's implementation to compute
If we do that then this confirms the difference explained above: If we exclude the last window, we get antropy's counts of I'm actually not sure which approach is correct. Do we have to exclude the last window in our computation? If so, why? I think we shouldn't, but I do see the argument for "if we do then we have the same number of windows for Edit: I have excluded counts for the last |
Hey @FirefoxMetzger — Thanks for the deep dive into this, and apologies about my delayed response. To be honest, I am not sure whether the last window should be excluded or not. Looking at the neurokit2 implementation, I see that the last window is also explicitly excluded: Perhaps @DominiqueMakowski might be able to chime in on this issue? FYI I have also ran some tests and for larger arrays (N>100), the exclusion/inclusion of the last window has very little impact on the final entropy value. PS2: neurokit2 uses |
If I remember, I think that's because sample entropy compares a time-delayed embedding to its m+1 version, for which the last row is unavailable (because of the bigger dimension), hence we remove it from the first so that it matches in terms of array shape. Hope that helps :) Regarding the homogenization, yeah I'd definitely agree that its a good idea (the discrepancies across the implementations kinda were the starting point for NeuroKit's version 😅 ) |
Thanks @DominiqueMakowski! @FirefoxMetzger I think that for now we can keep the removal of the last row, feel free to submit a PR with your improved numba implementation! |
@raphaelvallat Thanks for the update :) I won't get a chance to work on it this week, but should be able to get it going in the coming one. |
I recently implemented my own version of
_numba_sampen
as a C extension. Mainly because I did not know about this package, but also because I thought it would be good to learn how to write C extensions for numpy.From my benchmarks, it looks like my implementation is slightly faster than the current one, so I was wondering if you would be interested in a PR to update the kernel. It would also improve the memory requirement, as the current implementation allocates several copies of the array, whereas my kernel is constant (wrt. array size and window size).
Here is a benchmark figure:
And here is a pure python implementation of my kernel:
The text was updated successfully, but these errors were encountered: