Skip to content

ENH : add new interpolate_mark_bads function#9241

Merged
drammock merged 9 commits intomne-tools:mainfrom
agramfort:enh_interpolate_mark_bads
Apr 5, 2021
Merged

ENH : add new interpolate_mark_bads function#9241
drammock merged 9 commits intomne-tools:mainfrom
agramfort:enh_interpolate_mark_bads

Conversation

@agramfort
Copy link
Copy Markdown
Member

closes #9231

origin fit.
exclude : list | tuple
The channels to exclude from interpolation. If excluded a bad
channel will stay bad.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would rather kill this docstring entirely rather than have to continue to update it, I think it actually reduces maintenance burden

Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Some preliminary comments

.. versionadded:: 0.21
exclude : list | tuple
The channels to exclude from interpolation. If excluded a bad
channel will stay bads.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"will stay in bads", or "will stay bad"

@@ -0,0 +1,64 @@
"""Tools for interpolation data."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"""Tools for interpolation data."""
"""Tools for data interpolation."""



def interpolate_mark_bads(insts, good_fraction=0., copy=True):
"""Interpolate or mark bads consistently for a list of data.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"""Interpolate or mark bads consistently for a list of data.
"""Interpolate or mark bads consistently for a list of instances.

The list of instances (Evoked, Epochs or Raw) to consider
for interpolation. Each instance should have marked channels.
good_fraction : float
A float between 0 and 1 that specifies the fraction of time
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
A float between 0 and 1 that specifies the fraction of time
A float between 0 (default) and 1 that specifies the fraction of time

and then you'll need to re-PEP8-ify it because the line is probably too long

from ..epochs import BaseEpochs


def interpolate_mark_bads(insts, good_fraction=0., copy=True):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The default I think should be to always just mark all bad (this seemed to be the consensus when we discussed), so good_fraction=1..

A float between 0 and 1 that specifies the fraction of time
a channel should be good to be eventually interpolated for
certain instances. For example if 0.5, a channel which is
good more than half of the time will be interpolated in the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What you wrote here implies > but the conditional below (which operates on the opposite) is <. So the wording here should be changed to reflect the effective >= operation.

Suggested change
good more than half of the time will be interpolated in the
good at least half of the time will be interpolated in the

Copy link
Copy Markdown
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

I'm not so sure the name for the new function is the best. To me personally, it's a bit confusing or at least not very intuitive.

WDYT about something like equalize_bads(), for consistency with equalize_epoch_counts() and equalize_event_counts()?

@agramfort
Copy link
Copy Markdown
Member Author

agramfort commented Apr 4, 2021 via email

@drammock
Copy link
Copy Markdown
Member

drammock commented Apr 4, 2021

why not for equalize_bads

Summer we're talking about API names, WDYT about changing "good_fraction" to something like "interp_thresh"? (Since it's the threshold above which interpolation will happen instead of marking bad)

@hoechenberger
Copy link
Copy Markdown
Member

@drammock +1!!

@agramfort
Copy link
Copy Markdown
Member Author

agramfort commented Apr 4, 2021 via email

Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM +1 for merge, @drammock feel free to merge if you're happy

@drammock drammock merged commit 4080b43 into mne-tools:main Apr 5, 2021
@drammock
Copy link
Copy Markdown
Member

drammock commented Apr 5, 2021

thanks @agramfort!

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.

mne.preprocessing.interpolate_bads taking list of instances

4 participants