WIP: Flag ics

[codecov[bot]]

Codecov Report

Merging #117 (a8851e2) into main (4db5348) will increase coverage by 54.79%. The diff coverage is 96.59%.

:exclamation: Current head a8851e2 differs from pull request most recent head 8c2e4dc. Consider uploading reports for the commit 8c2e4dc to get more accurate results

@@             Coverage Diff             @@
##             main     #117       +/-   ##
===========================================
+ Coverage   22.65%   77.44%   +54.79%     
===========================================
  Files           7       12        +5     
  Lines         565      909      +344     
===========================================
+ Hits          128      704      +576     
+ Misses        437      205      -232     

Impacted Files	Coverage Δ
pylossless/dash/topo_viz.py	83.69% <91.17%> (ø)
pylossless/_utils.py	100.00% <100.00%> (+73.33%)	:arrow_up:
pylossless/dash/tests/conftest.py	100.00% <100.00%> (ø)
pylossless/dash/tests/test_topo_viz.py	100.00% <100.00%> (ø)
pylossless/flagging.py	64.28% <100.00%> (+33.73%)	:arrow_up:
pylossless/pipeline.py	72.25% <100.00%> (+53.66%)	:arrow_up:

... and 4 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[christian-oreilly]

@scott-huberty OK. Getting back at this as a target to have this PR fixed and merged before our Friday call. Sorry for the lag.

[scott-huberty]

@scott-huberty OK. Getting back at this as a target to have this PR fixed and merged before our Friday call. Sorry for the lag.

Awesome thanks for getting to that before me : )

BTW/FYI- sorry I don't have screenshots on hand, but I viewed a topoplot of a clear blink component in an ica object, first in MNE and then in the qc-dashboard (generated by our plotly code). There is definitely something weird going on. A beautiful blink component (when you plot the component in MNE), looks like mumbo jumbo with our plotly topolot!

[christian-oreilly]

BTW/FYI- sorry I don't have screenshots on hand, but I viewed a topoplot of a clear blink component in an ica object, first in MNE and then in the qc-dashboard (generated by our plotly code). There is definitely something weird going on. A beautiful blink component (when you plot the component in MNE), looks like mumbo jumbo with our plotly topolot!

Yes... I had little doubts that there was an issue of that sort. The topomaps we were seeing were not what we would expect... I'll debug this with the rest.

I started by adding some tests. You might find the last commit interesting because it has a Dash test. These three tests works on my computer, but the last one (the Dash one) won't work for now on CI. It requires a cromedriver to be installed (and findable). The download, unzip, and move to a path with PATH would need to be added to the CI script. Also, this requires chrome to be installed and runnable... I don't know if the CI can do that (i.e., I am not sure it has a X server... if it is just a command line OS, I don't see how pytest will be able to launch chrome to make the test. To be tested...

[christian-oreilly]

Nevermind. It looks like CI with Dash test will not be complicated (https://community.plotly.com/t/dash-integration-testing-with-selenium-and-github-actions/43602/2). Just pushed a commit with that modif.

[scott-huberty]

Go @christian-oreilly go 🚀

[scott-huberty]

Thanks for tackling this and breaking the ice and the dash tests 🙏

Note that we could make a new GitHub actions yaml file, name it something like test_dashboard and run the tests from there, so that this shows up as it's own GitHub action when opening PR's.

[christian-oreilly]

Note that we could make a new GitHub actions yaml file, name it something like test_dashboard and run the tests from there, so that this shows up as it's own GitHub action when opening PR's.

Yeah... I guess we could do that. I have no objections about it, but I am not sure to see a strong advantage of it. Current tests work for the topoplot but fails for the pipeline:

collected 8 items pylossless/dash/tests/test_topo_viz.py ... [ 37%] tests/test_pipeline.py ..FF [ 87%] tests/test_simulated.py F [100%]

Do I need to rebase against some branch for these to work? I am not sure when these started to break. Do you think these have been broke by this specific PR?

[christian-oreilly]

Nevermind! We broke them I think with the changes in "manual" flags. I'll fix these.

[scott-huberty]

Nevermind! We broke them I think with the changes in "manual" flags. I'll fix these.

Yeah - just have to remove this line now that the 'manual' key is remoeved:

https://github.com/lina-usc/pylossless/blob/9160b94020ee22d63200d8116bc7fcad0c121bb2/tests/test_simulated.py#L188

[scott-huberty]

Also CC @Andesha

this PR is API changing.

After it's merged I'll try to add a function for converting flags to mne-bads so that people don't have to worry about custom scripting it.

[Andesha]

thanks for letting me know!

is this going to be pushed as a release?

[scott-huberty]

is this going to be pushed as a release?

Not immediately but We will cut a new release relatively soon I believe.

[christian-oreilly]

The latest IC topomaps seem more reasonable to me (more like patterns I would expect from IC topomaps):

[christian-oreilly]

I don't l know what the hell is wrong with GItHub but it keep saying that there was one pending reviewer (me) although I already approved that PR! Anyway, @scott-huberty do you want to review this before I merge once I fixed the linting?

[scott-huberty]

Sure I'll try to review before todays session! The find_breaks test is failing and I want to find out why

[scott-huberty]

The latest IC topomaps seem more reasonable to me

They still seem a little fishy to me (I'd expect at least one clear ocular looking topo from this very nice data). Have you checked any of these topos against what would be generated by mne.preprocessing.ica.plot_topomap?