Skip to content

Conversation

@eioe
Copy link
Contributor

@eioe eioe commented Apr 9, 2021

Related to PR #9124

To demonstrate possible usage of the to_data_frame method, I adapted an example that uses an EpochsTFR instance.
@dengemann WDYT?
@cbrnr since I'm mutilating your (very nice) example - you should have a veto before this goes out. 🙂
Thx!

Also, in the original PR I forgot to do the housekeeping in terms of updating latest.inc and names.inc. So I'm sneaking this in here.

@eioe eioe changed the title Tfr todf example [WIP] example for using to_data_frame method on EpochsTFR Apr 9, 2021
@cbrnr
Copy link
Contributor

cbrnr commented Apr 9, 2021

Will take a look next week!

@eioe
Copy link
Contributor Author

eioe commented Apr 9, 2021

tbh, I do not understand why it crashes building the docs now and the CircleCI output is unclear to me. Can you maybe help @drammock ?

@drammock
Copy link
Member

drammock commented Apr 9, 2021

tbh, I do not understand why it crashes building the docs now and the CircleCI output is unclear to me. Can you maybe help @drammock ?

Looks like you're hit by the problem that @GuillaumeFavelier is trying to fix in #9274

@eioe
Copy link
Contributor Author

eioe commented Apr 10, 2021

ah yes, I see, thanks @drammock

@eioe
Copy link
Contributor Author

eioe commented Apr 15, 2021

@cbrnr fyi, building the docs now worked. If you want you can take a look at the rendered result here:
https://27276-1301584-gh.circle-artifacts.com/0/dev/auto_examples/time_frequency/plot_time_frequency_erds.html
I'm all open for any kind of suggestions.

@eioe eioe changed the title [WIP] example for using to_data_frame method on EpochsTFR Example for using to_data_frame method on EpochsTFR Apr 16, 2021
@eioe eioe marked this pull request as ready for review April 16, 2021 09:05
@cbrnr cbrnr added this to the 0.23 milestone Apr 19, 2021
eioe and others added 2 commits April 21, 2021 13:13
Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@cbrnr
Copy link
Contributor

cbrnr commented Apr 21, 2021

@eioe can you ping me once you've resolved @drammock's comments? Then I will take a look.

@eioe
Copy link
Contributor Author

eioe commented Apr 22, 2021

@drammock I implemented all requested changes. Unfortunately, there's now a merging conflict that I don't understand. Can you point me into a direction how to fix it?

@drammock
Copy link
Member

@drammock I implemented all requested changes. Unfortunately, there's now a merging conflict that I don't understand. Can you point me into a direction how to fix it?

probably related to #9292. All the examples/tutorials were renamed to no longer have filenames that start with plot_ (they originally did so because of a default setting in sphinx-gallery). You'll need to put your changes into the newly-renamed file and delete the old plot_* file.

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

ok @eioe, I think this is my last round of comments! This is looking really good.

@eioe
Copy link
Contributor Author

eioe commented Apr 23, 2021

@drammock thanks again for the in-depth review and all the constructive suggestions. Learned a lot.
I implemented all of them.
Only, the original figures of the tutorial (the TFR plots) are now very small. I tried a lot to change their size (figsize=(...), fig.set_size_inches(...), fig.tight_layout(), fig.adjust_subplots(...), and various combinations) but without success. So I put it back in it's original state. Help is appreciated.
Running time of the script on CircleCI seems to be ~24s - if I interpret that correctly. Is that too long for an example? (Original version, before my changes, was at 7s. 😐 )

@drammock
Copy link
Member

the original figures of the tutorial (the TFR plots) are now very small.

This was not caused by you; they are small in the current website too: https://mne.tools/dev/auto_examples/time_frequency/time_frequency_erds.html (they look bigger in the stable docs because that site has a wider width for the main content of the page).

This happens because if a code cell generates more than 1 figure, sphinx-gallery will always put them into two columns. If you want to force plots to be full width instead of side-by-side, you can do something like this:

fig1 = raw.plot()

###############################################################################

fig2 = raw.plot_psd()

That approach is harder with the code cell in this example because the 2 figures are generated by iterations of a for-loop, so you can't easily separate out the second plotting call from the first one.

@cbrnr I'm happy with this one, your turn to review/merge :)

Copy link
Contributor

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Very nice addition and beautiful plots! I have a couple of (minor) comments that I think should be addressed before merging.

Comment on lines +126 to +127
df['channel'].cat.reorder_categories(['C3', 'Cz', 'C4'], ordered=True,
inplace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be an ordered categorial variable? There isn't really any order imposed by the channels so I was wondering if we could drop ordered=True and it still works (I assume you want to change the alphabetical order). Can you try?

Copy link
Member

Choose a reason for hiding this comment

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

It is actually already a categorical variable (this happens automatically in our dataframe export code), here we're just changing the order. it determines what order the columns appear in the seaborn FacetGrid. To me it made sense to have Cz in the middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was also my thinking - also for consistency with the time-freq plots above. I suggest keeping it in this order but can of course also take it out. Your call @cbrnr 🙂

Comment on lines 135 to 144
def map_bands(freq):
for band, (low_lim, high_lim) in freq_bands.items():
if low_lim <= freq <= high_lim:
return band


df['band'] = pd.Categorical(
df['freq'].map(map_bands),
categories=['beta', 'alpha', 'theta', 'delta'],
ordered=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this binning be simplified with https://pandas.pydata.org/docs/reference/api/pandas.cut.html?

Copy link
Member

Choose a reason for hiding this comment

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

yes! I knew there was a function for that but I couldn't remember its name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm doing this now:

freq_bounds = {'_': 0,
               'delta': 3,
               'theta': 7,
               'alpha': 13,
               'beta': 35,
               'gamma': 140}

df['band'] = pd.cut(df['freq'], list(freq_bounds.values()),
                    labels=list(freq_bounds.keys())[1:])

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

list(freq_bounds.keys()) is the same as list(freq_bounds), FYI. Otherwise looks fine

Copy link
Member

Choose a reason for hiding this comment

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

actually looking at the rendering on CircleCI I wonder if you got the call to pd.cut correct? there's an empty row at the bottom of the plot and the top row looks really noisy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh, that's what you get when "quickly finishing something friday night" ... thanks for spotting.


there's an empty row at the bottom of the plot

Putting this fixed the empty row for gamma frequencies:
df['band'] = df['band'].cat.remove_unused_categories()


the top row looks really noisy

I think, that's because now only two freqs (2 & 3 Hz) go into this plot and the # of reps for the CI bootstrap is very low. It's a tiny bit better with 3 freqs (aka, having the boundary between delta and theta at 4 Hz instead) - but not so much.
I think that the actual call to to pd.cut is ok. I also tested it against the previous mapping method. Results were identical.

Comment on lines 168 to 178
g = sns.FacetGrid(df_mean, col='condition', col_order=['hands', 'feet'],
margin_titles=True)
g = (g.map(sns.violinplot, 'channel', 'value', 'band', n_boot=10,
palette='deep', order=['C3', 'Cz', 'C4'],
hue_order=['delta', 'theta', 'alpha', 'beta'])
.add_legend(ncol=4, loc='lower center'))

g.map(plt.axhline, **axline_kw)
g.set_axis_labels("", "ERDS (%)")
g.set_titles(col_template="{col_name}", row_template="{row_name}")
g.fig.subplots_adjust(left=0.1, right=0.9, top=0.9, bottom=0.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

The violins are kind of hard to see - can you maybe disable their black outlines (and use no outline instead)?

Copy link
Contributor Author

@eioe eioe Apr 23, 2021

Choose a reason for hiding this comment

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

I decreased the linewidth. Looks better to me than no outline at all. Agree? (Lates CircleCI rendering)

eioe and others added 10 commits April 23, 2021 19:11
fix doc string layout

Co-authored-by: Clemens Brunner <clemens.brunner@gmail.com>
Co-authored-by: Clemens Brunner <clemens.brunner@gmail.com>
Co-authored-by: Clemens Brunner <clemens.brunner@gmail.com>
Co-authored-by: Clemens Brunner <clemens.brunner@gmail.com>
@eioe
Copy link
Contributor Author

eioe commented Apr 23, 2021

Thanks @cbrnr for the review. Fixes, comments and suggestions are commited.

@drammock
Copy link
Member

CircleCI failure is real:

/home/circleci/project/doc/auto_examples/time_frequency/time_frequency_erds.rst:503: WARNING: py:obj reference target not found: time_format=None

probably you used single-backticks but needed 2 backticks on each side.

@larsoner larsoner merged commit dece679 into mne-tools:main Apr 26, 2021
@larsoner
Copy link
Member

All looks good, thanks @eioe !

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