Fixing topomaps.

[christian-oreilly]

Fix #129

[christian-oreilly]

These modifs seems to fix most of the problem... but there appear to still have some issues. Comparing with MNE topomaps using:

import mne
fname = "/Users/christian/Code/pylossless/pylossless/assets/test_data/sub-s02/eeg/sub-s02_task-faceO_ica1_ica.fif"
ica =  mne.preprocessing.read_ica(fname)
ica.plot_components()

we obtain:

Compared to what we get with TopoViz:

The first 5 seem to be the same, with an inversion in the camp (red becomes blue)... then, some seem to correspond but not all, and not for the same index...

[scott-huberty]

I pushed a couple commits to get this PR over the line, our topoplots now match those produced by MNE. Using one of our pylossless test files:

fpath = Path(".") / "pylossless" / "pylossless" / "assets" / "test_data" / "sub-s01" / "eeg"
ica_fpath = fpath / "sub-s01_task-faceO_ica2_ica.fif"
ica = mne.preprocessing.read_ica(ica_fpath)
ica.plot_components(slice(0,20))

MNE:

and then selecting sub-s01_task-faceO_eeg.edf from the dropdown menu of the dashboard launched by pylossless_qc:

@christian-oreilly just give me a thumbs up if you agree that this looks like it solves the problem. FYI see commit 3dd5988 for the code I added.

Then there's just a failure in test_GridTopoPlot that needs to get worked out, and maybe we should try to add a specific test that assures that a topo of one component looks like one produced by MNE.

[scott-huberty]

minor suggestions

[scott-huberty]

Note: that there is also a typo here (color should be "black" not "back") that we should fix before merging.

[christian-oreilly]

Nice job. Yes, I agree, these topomaps now seem to be identical. I suspect that the CI issue is the same one discussed in the context of the "rejection policy" PR, which seems to have nothing to do with either PR.

[scott-huberty]

Nice job. Yes, I agree, these topomaps now seem to be identical. I suspect that the CI issue is the same one discussed in the context of the "rejection policy" PR, which seems to have nothing to do with either PR.

The test failure is related to this PR: It was caused by the change committed to pylossless/dash/tests/test_topo_viz.py, here

[christian-oreilly]

The test failure is related to this PR: It was caused by the change committed to pylossless/dash/tests/test_topo_viz.py, here

Oh... sorry about that. I just assumed. I should not have assumed without taking some time to check.

I must say that I don't remember why I changed it that way. I think our tests should guide us there... I'd go with your reverting of this change if it allows all tests to pass. We can always change this back later (adding corresponding tests) if we come up with a use case that requires this change...

[scott-huberty]

The test failure is related to this PR: It was caused by the change committed to pylossless/dash/tests/test_topo_viz.py, here

Oh... sorry about that. I just assumed. I should not have assumed without taking some time to check.

I must say that I don't remember why I changed it that way. I think our tests should guide us there... I'd go with your reverting of this change if it allows all tests to pass. We can always change this back later (adding corresponding tests) if we come up with a use case that requires this change...

No worries, it has been quite some time since we worked on this! I already checked, and reverting the specific change to test_topo_viz fixes the failure. Per my comment above, it makes since to me that the input to GridTopoPlot should be a list of dict, so the code as it was before makes since to me. I'll revert the change.

[codecov]

Codecov Report

Merging #130 (f264469) into main (662d837) will decrease coverage by 9.30%.
Report is 3 commits behind head on main.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
- Coverage   77.63%   68.33%   -9.30%     
==========================================
  Files          12       14       +2     
  Lines         921      960      +39     
==========================================
- Hits          715      656      -59     
- Misses        206      304      +98     

Files	Coverage Δ
pylossless/dash/utils.py	100.00% <100.00%> (ø)
pylossless/dash/topo_viz.py	55.20% <50.00%> (-28.49%)	⬇️

... and 6 files with indirect coverage changes

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

[scott-huberty]

I'm trying to get our code coverage to be ever increasing and there are missed lines in the added code. Will have to loop back to this when we have time.

[christian-oreilly]

@scott-huberty Can we merge this? I need it to be merged before forking this repo to start splitting the dash components into CONIE.

[scott-huberty]

@christian-oreilly sure, but can you commit my suggestions before?

Note that dash/tests/test_topoviz/test_TopoViz is failing, IIRC it was a chrome driver issue. I marked it for skip in #144

[christian-oreilly]

Agreed and committed suggestions.