search

lina-usc / pylossless

🧠 EEG Processing pipeline that annotates continuous data
https://pylossless.readthedocs.io/en/latest/
MIT License
15 stars 8 forks source link

Adding the rejection policy. #138

Closed christian-oreilly closed 8 months ago

christian-oreilly commented 12 months ago

Fix #115

christian-oreilly commented 12 months ago

I am happy with that. The functionalities it provides are basic, but it helps structure PyLossless. @scott-huberty You can review whenever convenient.

christian-oreilly commented 12 months ago

That is strange. All the stuff about deleting _check_sfreq (actually, all the modifs in pipeline.py) is not something I made so I am not so sure how it got there. I created the branch from the main and completed the whole thing within the span of an evening... I am not sure how it interfered with these changes... Do these changes ring a bell for you @scott-huberty ?

christian-oreilly commented 12 months ago

Nevermind... I just reverted the commit for this file. Strange though...

christian-oreilly commented 12 months ago

@scott-huberty The test that fails is currently in pylossless/tests/test_simulated.py . I did not change this file. Maybe it is related to this weird commit that I needed to revert? Would you mind having a look at it when you have some time? There is no rush for me.

christian-oreilly commented 10 months ago

@scott-huberty These CI issues are unrelated to this PR. Since you do not have time to look at it now and it has been blocking this PR for a while, I propose we merge this one in regardless so that we can use this functionality.

scott-huberty commented 10 months ago

@scott-huberty These CI issues are unrelated to this PR. Since you do not have time to look at it now and it has been blocking this PR for a while, I propose we merge this one in regardless so that we can use this functionality.

Thanks for starting this! I'm really sorry that I've not had time to review this. Things are winding down with my internship so I can help get this through this week.

I totally agree that unrelated test failures can be fixed in subsequent PR's, but there was already a typo in this PR that caused a bug, which wasn't apparent until I pulled this branch and ran the code locally. I've pointed out the typo and also proposed tests so that we can see right away if the code works as expected.

scott-huberty commented 10 months ago

Also if you're busy and won't have time to work on this soon, and are okay with me pushing directly, I can take a whack it it later this week!

christian-oreilly commented 10 months ago

Additional comment from working on the PyLossless paper: "we should clarify the 1-100 Hz bandpass filtering and notch filtering as "flagging filters" and the raw object output by the pipeline should not be modified by them (lossless). If these filters are to be used for analysis, then they should be applied by the rejection policy. This can be dealt seamlessly by, e.g., a binary argument "apply_flagging_filters", to the rejection policy class. "

codecov[bot] commented 9 months ago

Codecov Report

Merging #138 (788347a) into main (b0c6ad0) will increase coverage by 7.13%. The diff coverage is 90.00%.

:exclamation: Current head 788347a differs from pull request most recent head 23eff9c. Consider uploading reports for the commit 23eff9c to get more accurate results

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   71.60%   78.73%   +7.13%     
==========================================
  Files          18       21       +3     
  Lines        1074     1152      +78     
==========================================
+ Hits          769      907     +138     
+ Misses        305      245      -60
Files Coverage Δ
pylossless/__init__.py 100.00% <100.00%> (ø)
pylossless/config/__init__.py 100.00% <100.00%> (ø)
pylossless/config/config.py 91.30% <100.00%> (ø)
pylossless/conftest.py 100.00% <100.00%> (ø)
pylossless/dash/tests/test_topo_viz.py 92.50% <100.00%> (+12.50%) :arrow_up:
pylossless/datasets/__init__.py 100.00% <100.00%> (ø)
pylossless/dash/__init__.py 83.33% <80.00%> (-16.67%) :arrow_down:
pylossless/datasets/datasets.py 92.30% <92.30%> (ø)
pylossless/config/rejection.py 85.36% <85.36%> (ø)

... and 2 files with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

scott-huberty commented 8 months ago

@christian-oreilly in the interest of getting unstuck here I propose we merge this and #145 (merge in the conceptual sense, not literally a git merge). Basically, we merge this PR with the apply method as you wrote it, but we carry over the RejectionPolicy constructor I wrote in #145 (I think it's important for the RejectionPolicy class to have a constructor).

christian-oreilly commented 8 months ago

but we carry over the RejectionPolicy constructor I wrote in https://github.com/lina-usc/pylossless/pull/145 (I think it's important for the RejectionPolicy class to have a constructor).

Not sure what you mean by this. The version of that class I wrote in this PR has a constructor. Maybe I am misunderstanding your message. Anyway, fine with me to proceed as you prefer. Unfortunately, I'll have very little time between now and the end of the month... so don't feel you need to wait after me if you are confident with your changes.