Skip to content

ENH: Add the ids='all' as an option for mne.event.shift_time_events#6143

Merged
massich merged 12 commits intomne-tools:masterfrom
Nichalas:fix_shift_no_ids
Apr 17, 2019
Merged

ENH: Add the ids='all' as an option for mne.event.shift_time_events#6143
massich merged 12 commits intomne-tools:masterfrom
Nichalas:fix_shift_no_ids

Conversation

@Nichalas
Copy link
Copy Markdown
Contributor

I added this option so that I can call shift_time_events in this manner:

- events = mne.event.shift_time_events( events, ids=np.unique(events[:,2]),  tshift=trigger_offset,  tshift, sfreq=raw.info['sfreq'])
+ events = mne.event.shift_time_events( events, ids='all',  tshift=trigger_offset,  tshift, sfreq=raw.info['sfreq'])

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 12, 2019

Codecov Report

Merging #6143 into master will increase coverage by 0.37%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6143      +/-   ##
==========================================
+ Coverage   88.64%   89.01%   +0.37%     
==========================================
  Files         407      410       +3     
  Lines       73764    73891     +127     
  Branches    12233    12256      +23     
==========================================
+ Hits        65386    65774     +388     
+ Misses       5512     5222     -290     
- Partials     2866     2895      +29

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.

Any chance to add some small test? We might not have any tests for this function, in which case a 5-10 liner based on manually creating an ndarray should do it

mne/event.py Outdated
events[:, 0] += int(tshift * sfreq)
elif isinstance(ids, list):
for ii in ids:
events[events[:, 2] == ii, 0] += int(tshift * sfreq)
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.

A nice DRY change would be to change the above branch mask=slice(None) and this one mask=np.in1d(events[:,2],IDs), then do the += below.

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.

(on mobile so please forgive the errors, hopefully you get the idea)

@larsoner
Copy link
Copy Markdown
Member

@Nichalas I pushed a few commits to clean things a bit. Can you add an entry to the Changelog in whats_new.rst (including a link to your URL at the bottom)?

@larsoner larsoner changed the title add the ids='all' as an option for mne.event.shift_time_events ENH: Add the ids='all' as an option for mne.event.shift_time_events Apr 16, 2019
@agramfort
Copy link
Copy Markdown
Member

@Nichalas tell us if you approved these changes so we can merge. Thx

@Nichalas
Copy link
Copy Markdown
Contributor Author

@agramfort yes, I approve!

@larsoner
Copy link
Copy Markdown
Member

@Nichalas do you have a name/URL we should point to? We still need to update whats_new.rst

@massich massich merged commit aa7b3a0 into mne-tools:master Apr 17, 2019
@massich
Copy link
Copy Markdown
Contributor

massich commented Apr 17, 2019

thx @Nichalas !!

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.

4 participants