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

Additional option to flatten TMS artifact #6915

Merged
merged 16 commits into from
Dec 16, 2024

Conversation

fmamashli
Copy link
Contributor

Reference issue

I added 'constant' mode to the fix_stim_artifact which uses the baseline to flatten the TMS pulse artifact.

What does this implement/fix?

It provides smoother signal relative to the 'linear' and 'window' mode.

mne/transforms.py Outdated Show resolved Hide resolved
@fmamashli fmamashli force-pushed the flat_artifact branch 2 times, most recently from ec95998 to 7833a11 Compare October 4, 2019 18:16
mne/preprocessing/stim.py Outdated Show resolved Hide resolved
mne/preprocessing/stim.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #6915 into master will increase coverage by 0.1%.
The diff coverage is 94.73%.

@@            Coverage Diff            @@
##           master    #6915     +/-   ##
=========================================
+ Coverage   89.55%   89.65%   +0.1%     
=========================================
  Files         430      430             
  Lines       76952    76985     +33     
  Branches    12549    12552      +3     
=========================================
+ Hits        68915    69023    +108     
+ Misses       5215     5149     -66     
+ Partials     2822     2813      -9

@jasmainak
Copy link
Member

@fmamashli can you update what's new after addressing my small comment?

mne/preprocessing/stim.py Outdated Show resolved Hide resolved
mne/preprocessing/stim.py Outdated Show resolved Hide resolved
@fmamashli
Copy link
Contributor Author

@fmamashli can you update what's new after addressing my small comment?

I checked if the mean during baseline is equal to the data from the TMS artifact time interval. I only checked one sample. I think that should be fine.

@jasmainak
Copy link
Member

CIs are failing

@jasmainak
Copy link
Member

Don't forget to update the latest.inc file to advertise your change in the next release

@fmamashli
Copy link
Contributor Author

CIs are failing

What is Cls?

@larsoner
Copy link
Member

larsoner commented Oct 8, 2019

The status icons toward the bottom of this PR page. For example from a few commits ago, Travis CI was unhappy:

https://travis-ci.org/mne-tools/mne-python/builds/594772600?utm_source=github_status&utm_medium=notification

@@ -55,10 +63,13 @@ def fix_stim_artifact(inst, events=None, event_id=None, tmin=0.,
Start time of the interpolation window in seconds.
tmax : float
End time of the interpolation window in seconds.
mode : 'linear' | 'window'
baseline: None or tuple of length 2
When mode = 'constant', baseline is required
Copy link
Member

Choose a reason for hiding this comment

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

You should allow (None, 0)

s_start = int(np.ceil(inst.info['sfreq'] * tmin))
s_end = int(np.ceil(inst.info['sfreq'] * tmax))
if mode == "constant" and baseline is None:
raise ValueError('Please provide the baseline for mode="constant"')
Copy link
Member

@jasmainak jasmainak Oct 10, 2019

Choose a reason for hiding this comment

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

use the function _check_baseline to make sure that base_t1 and base_t2 provided by the user are good

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit more complicated than this because this function can also operate on raw instances.

Given this I'm just going to make it take a tuple of length 2 containing floats only, a follow-up PR could make it support None for Epochs if it makes sense. But in the meantime I don't mind the extra explicitness!

Base automatically changed from master to main January 23, 2021 18:26
@larsoner larsoner added this to the 1.10 milestone Dec 16, 2024
* upstream/main: (3619 commits)
  MAINT: Update code credit [ci skip] (mne-tools#13029)
  MAINT: Credit [ci skip]
  Add `raw.rescale` (mne-tools#13018)
  MAINT: Automerge for bot PRs (mne-tools#13015)
  MAINT: Also update fsaverage in sample (mne-tools#13009)
  Revert "[ENH] Add fig.mne container for colorbar (mne-tools#13019)"
  [ENH] Add fig.mne container for colorbar (mne-tools#13019)
  BUG: Better Wayland and open-source 3D support (mne-tools#13012)
  DEP: Support Python 3.13 (mne-tools#13021)
  MAINT: Audit CIs using zizmor (mne-tools#13011)
  [BUG] Fix simple documentation bug (mne-tools#13020)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13017)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13001)
  expose transform_to_head in API docs (mne-tools#13010)
  Fix cnt response (mne-tools#13007)
  ENH: Add round-trip channel name saving (mne-tools#13003)
  update governance (mne-tools#12896)
  pin selenium and stop filtering its warning (mne-tools#13000)
  DOC: fix return value doc of inst.get_montage() (mne-tools#12995)
  remove some ._filenames uses in favor of .filenames (mne-tools#12996)
  ...
@larsoner
Copy link
Member

Merged with main and pushed a tiny commit to fix some final doc issues, marking for merge-when-green, thanks in advance @fmamashli !

@larsoner larsoner modified the milestones: 1.10, 1.9 Dec 16, 2024
@larsoner larsoner enabled auto-merge (squash) December 16, 2024 17:27
@larsoner
Copy link
Member

OpenNeuro is having a bad day so I'm going to manually merge

@larsoner larsoner disabled auto-merge December 16, 2024 18:06
@larsoner larsoner merged commit 610ec2a into mne-tools:main Dec 16, 2024
25 of 30 checks passed
qian-chu pushed a commit to qian-chu/mne-python that referenced this pull request Jan 20, 2025
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.

3 participants