Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add ERDS topomap plotting #278

Merged
merged 6 commits into from
Feb 16, 2022
Merged

Add ERDS topomap plotting #278

merged 6 commits into from
Feb 16, 2022

Conversation

hofaflo
Copy link
Contributor

@hofaflo hofaflo commented Feb 10, 2022

To-Do:

There's a lot of code duplication in dialogs/erds.py, but since we want to add significance masks to ERDS maps soon, I'd postpone refactoring to when that is done.

@hofaflo hofaflo marked this pull request as ready for review February 11, 2022 10:44
@hofaflo hofaflo force-pushed the erds-topomap branch 2 times, most recently from cb9905a to 01fb4e8 Compare February 11, 2022 14:00
@cbrnr
Copy link
Owner

cbrnr commented Feb 13, 2022

Do you want me to merge this one or is there something you wanted to refactor?

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 13, 2022

This one's ready, I'd prefer to refactor once you are happy with #279 interface- and functionality-wise :)

@cbrnr
Copy link
Owner

cbrnr commented Feb 13, 2022

I just tested this PR and I have three comments:

  1. The menu entry should be disabled if there are no locations (montage) associated with the data.
  2. What is calculated here? The mean of the frequency and time range? Why is step size important then (for the user)?
  3. How are the plots scaled? The colorbar is labeled with "AU", but since we're computing ERDS it should be labeled as such. Also, the lower boundary should be -100% (or -1, I don't even know if we're showing values in percent or not). And the upper boundary is usually between 2.5 and 3.5, maybe this should be settable (with an option to determine the maximum from the data)?

Do you have any good example data where we can test this? Maybe the data from the ERDS maps tutorial? But here the channel labels end with dots and there's no easy way to remove them in MNELAB...

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 14, 2022

I'm lacking ERDS knowledge, so I think it would be best if we discuss this on tuesday in "person" 😅

I used the data from your BCI-event ERDS Tutorial for trying things out.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 15, 2022

The cmap used in viz.plot_erds_topomaps is now the same as in viz.plot_erds. #299 will change viz.plot_erds to use cnorm. This is not applicable here as mne.viz.plot_tfr_topomap does not have a cnorm parameter 🤔

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

Then I guess we should include center_cmap in MNELAB for now. The behavior of TwoSlopeNorm has changed in recent Matplotlib versions anyway, the colorbar has a weird scaling which I don't like (see my question here).

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

The dialog is also missing edits to select the time range that should be averaged.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 15, 2022

The dialog is also missing edits to select the time range that should be averaged.

Strange, it's there for me 🤔
image

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

Maybe I forgot to pull. I though VS Code does this automatically.

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

No wait, it's not there! The time range is for computing the complete map. We need another time selection for the average.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 15, 2022

The map is always computed with the entire epoch duration (there's no time parameter here). The time range that's entered in the dialog is then applied here.

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

I see, OK, that makes sense.

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

Do you want to include center_cmap in this PR?

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 15, 2022

Do you want to include center_cmap in this PR?

I'm not sure what you mean 🤔

@cbrnr
Copy link
Owner

cbrnr commented Feb 15, 2022

Include it in MNELAB because it will be gone from MNE.

@hofaflo
Copy link
Contributor Author

hofaflo commented Feb 15, 2022

Aaaah 🙈 done!

This was referenced Feb 16, 2022
@cbrnr cbrnr merged commit 7f3fbc6 into cbrnr:main Feb 16, 2022
@cbrnr
Copy link
Owner

cbrnr commented Feb 16, 2022

Thanks @hofaflo!

@hofaflo hofaflo deleted the erds-topomap branch February 16, 2022 15:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants