lina-usc / pylossless

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

FIX: fixed bug in marksarray2flag & pipeline.flag_ch_sd #53

Closed scott-huberty closed 1 year ago

scott-huberty commented 1 year ago

fixes #45 Closes #54 Closes #52 this was a nasty one. as far as I can see there are several problems - Calling the wrong flag_dim in the function call of marks_array2flags, and on top of that averaging across the wrong dimension in marks_array2flags

This fixes the issue for flagging epochs, but I assume the same bug still exists when usingmarks_array2flags to flag channels. UPDATE: I fixed the bug for both the epochs and channels dimension and added a test

UPDATE.. and ICs..

scott-huberty commented 1 year ago

after running the simulated raw object in tests/test_simulated

Screen Shot 2023-03-12 at 4 58 17 PM

Screen Shot 2023-03-12 at 4 59 47 PM

scott-huberty commented 1 year ago

After the changes already made here, and further fixes to flag_ic_sd1 after #54 with @jadesjardins, here is what face13/sub-s01 looks like..

image

epochs are flagged both after flag_ch_sd and after flag_epoch_ic_sd1.

image

Looking better, though we need to further build out the testing suite with our simulate file, so we can make sure the pipeline is behaving as we expect it to.

scott-huberty commented 1 year ago

DO NOT merge this branch until rebased with main (conflicts likely)

scott-huberty commented 1 year ago

@christian-oreilly I already merged main into this branch so I'll leave it to you to merge after review and tests pass. You can request changes or make them yourself if need be. Have a good week!

scott-huberty commented 1 year ago

test is passing on my local repeatedly. No idea why it is not passing CI.

scott-huberty commented 1 year ago

Whatever it is. A noise difference of 7e-8 makes the difference between not being flagged at all by either flag_outlier_chs and flag_ch_sd_ch to be flagged entirely by flag_outlier_chs. More to @christian-oreilly 's point that flag_ch_sd_ch may not functionally be doing anything that flag_outiler_chs isn't already doing. The noise window for being flagged by flag_ch_sd_ch but NOT flag_outliers_ch is tiny.. again 7e-8.

So I wouldn't be surprised if there is some tiny difference in the operating system (windows vs linux) or pacakge version of numpy, on the random state, that is being picked up on by this test. Again its like we are trying to thread a needle.

scott-huberty commented 1 year ago

So....

I think the idea is that flag_outlier_chs should flag channels that are just bad throughout the whole recording (like channels that are not seated properly).

flag_ch_sd_ch should flag channels that are noisy for some but not all the time (default, more than 20% of epochs).

But we could test more to confirm that hypothesis.

Good to merge from my end.

scott-huberty commented 1 year ago

maintenance note: Removed issue #72 from the fixed list in the first comment - Since I just reverted the associated commit and that issue will be tackled in a separate PR.

scott-huberty commented 1 year ago

Maintenance note: added #52 to the closing list - This PR has refactored that function.