nbara / python-meegkit

🔧🧠 MEEGkit: MEG & EEG processing toolkit in Python
https://nbara.github.io/python-meegkit/
BSD 3-Clause "New" or "Revised" License
182 stars 50 forks source link

[NEW] Add dss_line_iter() #52

Closed maciekszul closed 2 years ago

maciekszul commented 2 years ago

Here's the iterative dss_line. Tried to conform the code to the convention you have used.

nbara commented 2 years ago

Ok I went ahead and made some changes:

codecov[bot] commented 2 years ago

Codecov Report

Merging #52 (812ebf1) into master (3105ad6) will increase coverage by 0.50%. The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   78.24%   78.75%   +0.50%     
==========================================
  Files          20       21       +1     
  Lines        2211     2306      +95     
==========================================
+ Hits         1730     1816      +86     
- Misses        481      490       +9     
Impacted Files Coverage Δ
meegkit/utils/testing.py 70.37% <70.37%> (ø)
meegkit/dss.py 89.92% <98.50%> (+7.98%) :arrow_up:
meegkit/__init__.py 100.00% <100.00%> (ø)
meegkit/utils/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3105ad6...812ebf1. Read the comment docs.

maciekszul commented 2 years ago

Ok I went ahead and made some changes:

  • fixed linting errors
  • added a basic test. This needs to be improved still

Thanks! I had a quick look at it yesterday, but was a bit busy.

maciekszul commented 2 years ago

I carved out 50 MB off of my data and will use it for testing.

nbara commented 2 years ago

Awesome. I have a few fixes that I am just finishing, namely:

Will push in the next hour or so.

I'll also update the branch in order to fix the CI (hopefully)

maciekszul commented 2 years ago
  • plot only once at the end (have an idea to show all the iterations nicely, but if it's less informative I will not do it)

plotting each iteration is vestigial to the initial development. Making it more compact will only help.

  • add some basic unit test with synthetic data

uploaded the small data packet tests/data/dss_line_iter_test_data.npy

nbara commented 2 years ago

uploaded the small data packet tests/data/dss_line_iter_test_data.npy

I started an example file. Ideally I would like it to show the difference between dss_line() and dss_line_iter().

Does your file benefit from dss_line_iter explicitely?

maciekszul commented 2 years ago

Not sure I understand. The difference is miniscule as you know. It removes component until the peak disappears.

data in the file before dss_line_iter

image

and after

image

nbara commented 2 years ago

Not sure I understand. The difference is miniscule as you know. It removes component until the peak disappears.

Precisely. For the user using MEEGkit, I would like the example to showcase the added value of dss_line_iter() compared to dss_line().

Therefore I would simply like to have an example for which dss_line does a bad/insufficient job, while dss_line_iter removes the noise completely

maciekszul commented 2 years ago

Ahh, I get it. Yeah, the data requires more than one component to be removed.

this is the psd after removing a single component.

image

nbara commented 2 years ago

I finished writing the example. What do you think?

Also is it OK if I cite your name and email in the code?

maciekszul commented 2 years ago

I finished writing the example. What do you think?

just pulled it and ran the tests. all looks pretty good to me.

Also is it OK if I cite your name and email in the code?

of course.

nbara commented 2 years ago

Cool, thanks @maciekszul