Closed eort closed 2 years ago
Thanks @eort. I have looked at the changes only quickly, but I can already tell you that the CI do not pass any more (you can check that locally by running make pep
from meegkit's root directory)
I think only a subset of those changes are beneficial (the "inplace" recentring for instance). Some others I am less sure about (for instance when you compute X-X_filt
4 times in the dss_line()
code).
I will try to post proper benchmarks (computation time and memory consumption) soon to get to the bottom of this.
Yeah I wasn't sure about those ones either, but for me saving memory was more critical than saving time. In any case, looking forward to those benchmarks!
(Hope CI is happier now)
(Hope CI is happier now)
Bah, I meant run pytest --noplots
to run the unit tests, which are broken (due to the changes in demean, apparently). Sorry!
I'll post the benchmarks shortly
Ok, I've run a memory profiler to assess changes.
Using simulated data with 100 channels and 1e6 time points @ 200 hz.
Memory:
Computation time:
Memory:
Computation time:
Okay, that is surprising.
That's it, right?
Pytest is passing now locally. The issue was the change to matrix.py
I've made some changes. The only thing I reversed from your code is the multiple X-X_filt, which cannot possibly beneficial in terms of computations.
If this is ok with you then let's merge this into #57
Sure, sounds good.
Here the promised PR. I don't have a good estimate of the saved memory (as it never worked with the original code) but the gain is at least 50% (reduced from far more than 24gb to 15gb at the max
In a nutshell:
fft
results, but feed it directly intoifft
Generally, my motivation was quite selfish. I wanted to make the algorithm work for my data. So, while I obviously tried to not break anything, there is a chance that for other data things might not work anymore. Particularly, the edit in
matrix.py
I suggested, because I simply don't understand why the code was what was. The new version works for my purposes, but perhaps not for others.If the PR is of any use to you, I can try to add a few tests.
Overall, the results looks really nice, I think:
ps. I am not the most versatile github user, so sorry that this is a separate PR and not an upgrade to #57 (if this was desirable)