Skip to content

MRG, FIX: make labels in epochs.plot start at 0, fixes issue #7260#7299

Merged
larsoner merged 6 commits intomne-tools:masterfrom
SophieHerbst:epochs_plot
Feb 11, 2020
Merged

MRG, FIX: make labels in epochs.plot start at 0, fixes issue #7260#7299
larsoner merged 6 commits intomne-tools:masterfrom
SophieHerbst:epochs_plot

Conversation

@SophieHerbst
Copy link
Copy Markdown
Contributor

@SophieHerbst SophieHerbst commented Feb 8, 2020

Makes epochs.plot() start at 0 instead of 1 (issue #7260).
Note: a bug with spyder (#6528) prevents me from testing what happens after closing the interactive plot. I checked that the indices returned for manually selected epochs start with 0, too.

Closes #7260

ax.set_xticks(ticks)
ax2.set_xticks(ticks[:n_epochs])
labels = list(range(1, len(ticks) + 1)) # epoch numbers
labels = list(range(0, len(ticks))) # epoch numbers
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.

This can be simplified to list(range(len(ticks))) :) But probably a matter of taste!

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 8, 2020

Codecov Report

Merging #7299 into master will decrease coverage by 0.87%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7299      +/-   ##
==========================================
- Coverage   89.82%   88.94%   -0.88%     
==========================================
  Files         447      447              
  Lines       80694    80856     +162     
  Branches    12876    12908      +32     
==========================================
- Hits        72482    71918     -564     
- Misses       5385     6145     +760     
+ Partials     2827     2793      -34     

mne/epochs.py Outdated
Comment on lines +1255 to +1258
logger.info('Dropped %d epoch%s: %s' % (count, _pl(count),
np.array2string(np.sort(try_idx),
precision=0,
separator=',')))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hoechenberger: how about this part (formatting and array to string) – is there a more efficient way to do it?

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 have done:

', '.join(map(str, np.sort(try_idx))

rather than np. array2string

Copy link
Copy Markdown
Member

@hoechenberger hoechenberger Feb 8, 2020

Choose a reason for hiding this comment

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

I've never used array2string and I don't really like map, so my approach would've been based on a list comprehension:

', '.join([str(i) for i in np.sort(try_idx)])

But that's not really shorter nor easier to read than your array2string approach, so…

@SophieHerbst
Copy link
Copy Markdown
Contributor Author

I will need help with fixing those tests. no idea what they want.

Copy link
Copy Markdown
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.

also don't forget to update latest.inc file @SophieHerbst
thanks!

mne/epochs.py Outdated
Comment on lines +1255 to +1258
logger.info('Dropped %d epoch%s: %s' % (count, _pl(count),
np.array2string(np.sort(try_idx),
precision=0,
separator=',')))
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 have done:

', '.join(map(str, np.sort(try_idx))

rather than np. array2string

@SophieHerbst
Copy link
Copy Markdown
Contributor Author

@agramfort I don't know what an .inc file is, nor where to find it.

@hoechenberger
Copy link
Copy Markdown
Member

hoechenberger commented Feb 8, 2020

@SophieHerbst It's the changelog:
https://github.com/mne-tools/mne-python/blob/master/doc/changes/latest.inc

fig = epochs.plot(scalings=None, title='Epochs')
ticks = [x.get_text() for x in fig.axes[0].get_xticklabels()]
assert ticks == ['1']
assert ticks == ['0']
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@agramfort is this correct? I don't fully understand what it should do, but it broke the circle-ci.

@hoechenberger
Copy link
Copy Markdown
Member

hoechenberger commented Feb 9, 2020

@SophieHerbst You will want to add youself to doc/changes/names.inc as well :) You need to spell your name exactly as you did in latest.inc, i.e. add sth like:

.. _Sophie Herbst: https://github.com/SophieHerbst

(but you may also insert a link to a personal website instead of your GitHub profile if you prefer)

I suppose this will fix Circle

@SophieHerbst
Copy link
Copy Markdown
Contributor Author

Done, thanks @hoechenberger!

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.

Otherwise LGTM +1 for merge

@larsoner larsoner changed the title make labels in epochs.plot start at 0, fixes issue #7260 MRG, FIX: make labels in epochs.plot start at 0, fixes issue #7260 Feb 10, 2020
@larsoner larsoner merged commit be4dfac into mne-tools:master Feb 11, 2020
@larsoner
Copy link
Copy Markdown
Member

Thanks @SophieHerbst !

AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
…s#7260 (mne-tools#7299)

* make labels in epochs.plot start at 0

* try to fix error in test_epochs_plot

* change array to string formatting

* edit latest.inc

* add name

* correct update latest.inc
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
…s#7260 (mne-tools#7299)

* make labels in epochs.plot start at 0

* try to fix error in test_epochs_plot

* change array to string formatting

* edit latest.inc

* add name

* correct update latest.inc
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.

Minor: epoch counts in epochs.plot() starts with 1 instead of 0

4 participants