Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[MRG] Add viz of auto-noisy segment detection #145

Merged
merged 5 commits into from Jul 6, 2020

Conversation

hoechenberger
Copy link
Member

This is currently non-interactive (purely static viz) until we've decided how to best add interactivity.

@agramfort
Copy link
Member

Shall we aim to have this in the report ?

@hoechenberger
Copy link
Member Author

Shall we aim to have this in the report ?

Very good point. Yes I think we should. Need to adapt the pipeline to work with the new BIDSPath first, though, before proceeding with this PR!


# Figure title should not overlap with subplots.
fig.tight_layout(rect=[0, 0.03, 1, 0.95])
plt.show()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not in MNE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jasmainak, so the code in this PR is based on what can be found in the Maxwell filter tutorial of MNE, see https://mne.tools/dev/auto_tutorials/preprocessing/plot_60_maxwell_filtering_sss.html#using-sss-and-maxwell-filtering-in-mne-python

You can also find an example plot there.

The reason this is currently not in "MNE proper" is that we haven't decided how to best implement this. So I'd consider the current code in this PR as temporary, until we have a better solution built right into MNE!

@jasmainak
Copy link
Member

can you share an example plot?

This is currently non-interactive (purely static viz) until we'vedecided
how to best add interactivity.
@hoechenberger
Copy link
Member Author

The figures are now automatically added to the report

Screenshot 2020-07-06 at 13 57 22

@hoechenberger hoechenberger changed the title Add viz of auto-noisy segment detection [MRG] Add viz of auto-noisy segment detection Jul 6, 2020
@agramfort
Copy link
Member

@hoechenberger can you share a rendered artifact on circle when you have one?

@hoechenberger
Copy link
Member Author

@agramfort Absolutely!

… I see it just failed, need to fix that :(

@hoechenberger
Copy link
Member Author

Also forgot to mention:
This now stores the scores returned by find_bad_channels_maxwell() as a JSON file, which is later used to retrieve those values when creating the figures.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful !

@agramfort agramfort merged commit 96c3329 into mne-tools:master Jul 6, 2020
@agramfort
Copy link
Member

thx @hoechenberger

cc @SophieHerbst

@hoechenberger
Copy link
Member Author

hoechenberger commented Jul 6, 2020

Feedback I got from non-neuroscience friends who know their sh*t about design and accessibility:

  • the gray vertical lines are awful
  • the colormap may not be optimal; should try out "inferno", "plasma", "magma"
  • interactivity typically adds a huge improvement in UX and usefulness to such figures

@hoechenberger hoechenberger deleted the auto-noisy-viz branch July 6, 2020 13:31
@agramfort
Copy link
Member

agramfort commented Jul 6, 2020 via email

@SophieHerbst
Copy link
Collaborator

Very nice @hoechenberger!
Do we now have a documentation somewhere of how the 'score' is computed (ideally in the bad channel detector function)?

@hoechenberger
Copy link
Member Author

hoechenberger commented Jul 6, 2020

Do we now have a documentation somewhere of how the 'score' is computed (ideally in the bad channel detector function)?

From the "Notes" section of the find_bax_channels_maxwell() doc:

Screenshot 2020-07-06 at 15 39 21

I tried to follow this through by stepping through the code line by line. I'm fine until including step 4; but step 5 either doesn't happen as described, or I'm misunderstanding something. What we do in the code in step 5, to my understanding, is we check if zscore(dk) > limit, where limit is user-supplied (and 7 by default).

What we return as the "noisy score" is zscore(dk).

@larsoner could you shed some light onto this? Am I missing something, or is the documentation outdated?

@hoechenberger
Copy link
Member Author

Friendly ping to @larsoner – could you help me here? :)

@larsoner
Copy link
Member

larsoner commented Jul 8, 2020

I tried to follow this through by stepping through the code line by line. I'm fine until including step 4; but step 5 either doesn't happen as described, or I'm misunderstanding something. What we do in the code in step 5, to my understanding, is we check if zscore(dk) > limit, where limit is user-supplied (and 7 by default).

A zscore(d) is just (d-mean)/std. So if you're checking d>mean+limit*std, equivalently you're checking (d-mean)/std>limit, i.e. zscore(d)>limit

@hoechenberger
Copy link
Member Author

hoechenberger commented Jul 8, 2020

A zscore(d) is just (d-mean)/std. So if you're checking d>mean+limit*std, equivalently you're checking (d-mean)/std>limit, i.e. zscore(d)>limit

Bazinga! You're absolutely right, thank you!

Do you think it would make sense to adjust the user-facing documentation to make this relation between limit and dk more obvious? i.e., state that d is being z-transformed to a z-score, and that limit is, effectively, given in z-scores too?

@larsoner
Copy link
Member

larsoner commented Jul 8, 2020

A parenthetical mentioning that this is equivalent to the zscore exceeding limit would be fine

@agramfort
Copy link
Member

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants