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

[NEW] Add dss_line_iter() #52

Merged
merged 8 commits into from
Nov 29, 2021
Merged

[NEW] Add dss_line_iter() #52

merged 8 commits into from
Nov 29, 2021

Conversation

maciekszul
Copy link
Contributor

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

@nbara nbara added the enhancement New feature or request label Nov 23, 2021
Copy link
Owner

@nbara nbara left a comment

Choose a reason for hiding this comment

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

Hi @maciekszul , thanks for this.

I made a few minor comments.

Before we merge, I would like to add a unit test, and maybe also an illustrated example for the documentation.

Also see the CI has failed because of linting errors (you can run make pep in the terminal to check this locally)

Do you have a data file we could use for this (it would need to be small in size to host it here). Otherwise I can try to generate some synthetic data.

meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
meegkit/dss.py Outdated Show resolved Hide resolved
@nbara nbara mentioned this pull request Nov 23, 2021
4 tasks
meegkit/dss.py Outdated Show resolved Hide resolved
@nbara
Copy link
Owner

nbara commented Nov 23, 2021

Ok I went ahead and made some changes:

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

@codecov
Copy link

codecov bot commented Nov 23, 2021

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%) ⬆️
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.

@nbara nbara mentioned this pull request Nov 24, 2021
@nbara nbara linked an issue Nov 24, 2021 that may be closed by this pull request
4 tasks
@nbara nbara changed the title ADD: added dss_line_iter (nbara/python-meegkit#50) [NEW] Add dss_line_iter() Nov 24, 2021
@maciekszul
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@nbara
Copy link
Owner

nbara commented Nov 24, 2021

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

  • 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)
  • add some basic unit test with synthetic data

Will push in the next hour or so.

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

@maciekszul
Copy link
Contributor Author

  • 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

- bump version
- fixed doc
- add `pytest --noplots` option
@nbara
Copy link
Owner

nbara commented Nov 24, 2021

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
Copy link
Contributor Author

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
Copy link
Owner

nbara commented Nov 24, 2021

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
Copy link
Contributor Author

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
Copy link
Owner

nbara commented Nov 26, 2021

I finished writing the example. What do you think?

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

@maciekszul
Copy link
Contributor Author

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
Copy link
Owner

nbara commented Nov 29, 2021

Cool, thanks @maciekszul

@nbara nbara merged commit ed28670 into nbara:master Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZapLine improvements
2 participants