WIP: Rework report – Looking for feedback! #9722

hoechenberger · 2021-09-07T19:13:25Z

Motivation

The goal of this PR is to make Report more versatile and to update the UI and functionality to better meet the expectations of new users in 2021.

Report used to have only few public methods; the primary way to use it was via parse_folder(). This, however, gave users little control over the generated output. In the MNE-BIDS-Pipeline, we would render most objects manually and then add them via add_figs_to_section() – a somewhat dull and cumbersome endeavour.

I therefore decided it would be nice to have numerous public methods that have an inherent understanding of various MNE-Python objects and could render them in a sensible way automatically, without requiring users to cook up their own plotting code.

What's new

Methods

add_epochs
add_evokeds
add_raw
add_stc
add_forward
add_trans
add_inverse
add_covariance
add_events
add_ssp_projs
add_code (arbitrary code blocks with syntax highlighting)
add_sys_info (output of mne sys_info)
add_bem (replaces add_bem_to_section)
add_figure (replaces add_figs_to_section)
add_image (replaces add_images_to_section)
add_slider (replaces add_slider_to_section)
add_html (replaces add_htmls_to_section)

"Data model"

The notion of "sections" is gone. Instead, all methods accept a parameter, tags, allowing users to assign an arbitrary number of tags to each added element.

UI

Table of contents now features a scrollspy, i.e. highlights the currently viewed section.
Elements are collapsible by clicking on their title.
New dropdown menu to select tags to display / hide.

Dependencies, custom JavaScript & CSS

I've updated jQuery and Bootstrap to the latest stable versions.
Most of our custom CSS could be removed by styling via Bootstrap classes.
I removed our custom slider, and created a new one based on Bootstrap's Carousel and Range components, with a little bit of custom JavaScript.
I removed jquery-ui, which we needed only for our custom slider.
I replaced jQuery calls with standard JS calls wherever possible.
Custom JavaScript now makes use of some basic ES6 features, which have been supported by all major browsers for 5–10 years now (specifically, I'm using fat arrow functions, the const and let keywords, and the spread operator)

MWE

# %%
from pathlib import Path
import mne


root_dir = Path(mne.datasets.sample.data_path())
meg_dir = root_dir / 'MEG' / 'sample'
fs_subjects_dir = root_dir / 'subjects'
fs_subject = 'sample'

raw_path = meg_dir / 'sample_audvis_filt-0-40_raw.fif'
ecg_proj_path = meg_dir / 'sample_audvis_ecg-proj.fif'
eog_proj_path = meg_dir / 'sample_audvis_eog-proj.fif'
trans_path = meg_dir / 'sample_audvis_raw-trans.fif'
cov_path = meg_dir / 'sample_audvis-cov.fif'
fwd_path = meg_dir / 'sample_audvis-meg-oct-6-fwd.fif'
inverse_path = meg_dir / 'sample_audvis-meg-oct-6-meg-inv.fif'
stc_path = meg_dir / 'sample_audvis-meg'

# Create epochs
raw = mne.io.read_raw(raw_path)
events = mne.find_events(raw, stim_channel='STI 014')
event_name_id_map = {
    'auditory/left': 1, 'auditory/right': 2, 'visual/left': 3,
    'visual/right': 4, 'smiley': 5, 'buttonpress': 32
}
epochs = mne.Epochs(
    raw=raw, events=events, event_id=event_name_id_map,
    tmin=-0.2, tmax=0.5, preload=True
)

evokeds = [
    epochs['auditory'].average(),
    epochs['visual'].average()
]

# %%
r = mne.Report(title='My Report')

r.add_raw(raw=raw_path, title='my raw data', tags=('raw',))
r.add_events(events=events, event_id=event_name_id_map, title='my events',
             sfreq=raw.info['sfreq'])
r.add_epochs(epochs=epochs, title='my epochs', tags=('epochs',))
r.add_evokeds(evokeds=evokeds[:2], noise_cov=cov_path,
              titles=['my evoked 1', 'my evoked 2'],
              tags=('evoked',))
r.add_ssp_projs(info=evokeds[0].info, projs=ecg_proj_path,
                title='my proj', tags=('ssp', 'ecg'))
r.add_ssp_projs(info=evokeds[0].info, projs=eog_proj_path,
                title='my proj', tags=('ssp', 'eog'))
r.add_covariance(cov=cov_path, info=raw_path, title='my cov')
r.add_trans(trans=trans_path, info=raw_path, title='my coreg',
            subject=fs_subject, subjects_dir=fs_subjects_dir)
r.add_bem(subject=fs_subject, subjects_dir=fs_subjects_dir,
          title='my bem')
r.add_forward(forward=fwd_path, title='my forward',
              subject=fs_subject, subjects_dir=fs_subjects_dir)
r.add_inverse(inverse=inverse_path, title='my inverse',
              subject=fs_subject, subjects_dir=fs_subjects_dir,
              trans=trans_path)
r.add_stc(stc=stc_path, title='my stc',
          subject=fs_subject, subjects_dir=fs_subjects_dir)
r.add_code(code=__file__, title='my code')
r.add_sys_info(title='my sysinfo')

r.save('/tmp/report.html', open_browser=True, overwrite=True)

Issues & todo

Low prio / do later

Clicking on a tag should hide all other content
Reconsider layout -> make columns truly responsive
On small screens, the TOC on the left should be hidden in a hamburger menu

The code, all in all, while functional, is still kind of messy. Still I'd appreciate code design feedback if you dare :) Otherwise I'm super happy about any feedback and thoughts on the redesigned UI and UX.

cc @dengemann @SophieHerbst @drammock @agramfort @sappelhoff @jasmainak

hoechenberger · 2021-09-07T19:17:09Z

mne/viz/utils.py

    # xmin and xmax
    vlines = [] if vlines is None else vlines
-    xticks = _trim_ticks(axes.get_xticks(), xmin, xmax)
+    xticks = _trim_ticks(axes.get_xticks(), round(xmin, 2), round(xmax, 2))


This improves the X-axis visualization of the GFP plots.

hoechenberger · 2021-09-07T19:23:14Z

We're also adding alt tags in many more places and I'm trying to follow a11y guidelines as good as I can

rob-luke · 2021-09-07T23:07:30Z

Reports are awesome and I'm currently addicted to them. The screen shot above looks great, and I like the tags filter feature.

It's unclear to me if you are proposing to depreciate add_figs_to_section or just revamp it? If you are depreciating, then how would one achieve the same functionality with your new methods?

jasmainak · 2021-09-08T02:27:50Z

Fantastic initiative! I second @rob-luke that I kind of like add_figs_to_section. For example, I could iterate over all the open figures in matplotlib and create a report that shows me all the figures from a piece of script.

I almost never need any of the other functions except for trans (and BEM). You need three views to get a good idea of the co-registration and messing with mayavi code to do that is somewhat non-trivial.

jasmainak · 2021-09-08T02:28:19Z

The tags feature looks awesome :-)

Should fix scrollspy issues

hoechenberger · 2021-09-08T07:20:39Z

Hello @jasmainak and @rob-luke, thanks for the positive feedback!

My goal is to retain backward-compatibility as much as possible, so my immediate next steps are

to ensure that all "old" add_*_to_section() methods still work, and internally simply map "section" to "tags"
add new methods that implement the same functionality, but follow the new method naming / signatures
deprecate the old methods, possibly with a longer-than-usual deprecation cycle if you think that makes sense

Does that sound okay to you?

rob-luke · 2021-09-08T07:50:49Z

Sounds great. Let me know when you want me to give it a spin locally.

sappelhoff

I haven't had the chance to use reports as much as I would have liked to, but the proposed changes sound good to me - and I'll try to give this a spin in my next project (couple of months/weeks from now though).

larsoner · 2021-09-08T15:51:17Z

Looks very promising!

We should definitely keep some variant of add_figs_to_section, being able to add custom items not supported directly in MNE is very useful. At UW we use a lot of custom report-related code -- once you think you have this backward compatibility worked out, I can try running our report generation code and fix any bugs I hit along the way (or report them here if you want to do it, but it's probably easier for me since I have the custom-report-generation code all set up locally).

Currently the diff is a pain to look at because it shows up as mne/report.py being deleted and mne/report/report.py being created. Could you at least for now restructure it so that there is mne/report.py with all the report code in it, and then have mne/_report/* for all the other stuff? That way reviewing the Python code changes will be much easier. Then right before merge merge, hopefully it's quick and easy to go back to the structure you have here just by changing a few paths (which I agree is better than a single monolithic report.py). If it's not clear what I mean, I can push a quick commit to do this.

hoechenberger · 2021-09-08T16:14:01Z

Currently the diff is a pain to look at because it shows up as mne/report.py being deleted and mne/report/report.py being created.

Yes, I'm surprised it is that way though, I thought git would track the content :( But even without this issue, the diff would be super painful, as I touch(ed) almost everything in report.py :S But I will see if I can make things slightly better by following your proposal.

I've locally gotten add_figs_to_section() to work again; it internally uses a new add_figs(). Still fighting with a last remaining bug.

While we're talking about this:
Are the scale, replace, and auto_close params actually of any use to you?

larsoner · 2021-09-08T16:35:57Z

Are the scale, replace, and auto_close params actually of any use to you?

I didn't even know those existed :) And based on what I assume they do, no, they are not. Scale and close can be done by the user when creating their figures. Replace maybe is useful but when scripting these things usually you don't need it.

Really @wmvanvliet might have some opinion on these, though, since I know he worked on class persistence maybe replace is indeed useful in his workflow(s)

hoechenberger · 2021-09-08T18:47:11Z

I've now resurrected add_figs_to_section(), which internally calls the new method add_figs().

MWE:

# %%
import mne
import matplotlib.pyplot as plt

r = mne.Report()

fig, ax = plt.subplots()
ax.plot([1,2,3], [1,2,3])

r.add_figs_to_section(
    figs=fig,
    captions='Old API, single fig',
    section='old-api',
    comments='this is a comment'
)

r.add_figs_to_section(
    figs=[fig, fig],
    captions=['Old API, multiple figs (1)',
              'Old API, multiple figs (2)'],
    section='old-api',
    comments=['a comment', 'this is another comment']
)


# New API, multiple figs
r.add_figs(figs=[fig, fig],
           titles=['New API (1)', 'New API (2)'],
           captions=['A caption', 'Another caption'],
           tags=('new-api',)
)


r.save('/tmp/report_figs.html', overwrite=True)

Rendered HTML can be downloaded from my Google Drive:
https://drive.google.com/file/d/1gmyZWApXOi9BhJovPDvEQVx3Sl26MxnA

@jasmainak @larsoner @rob-luke Does this have the functionality you need? Or did I mess something up for you?

Next up: fix the remaining add_*_to_section() methods!

larsoner · 2021-09-08T18:50:34Z

Rendered HTML can be downloaded from my Google Drive:

I would rather see the CircleCI build fixed, then we can review using the standard CI checking schemes. add_figs_to_section is used in tutorials/intro/70_report.py...

hoechenberger · 2021-09-08T18:53:45Z

I would rather see the CircleCI build fixed

Good point, coming right up! 🏃 🏃 🏃

drammock · 2021-09-08T19:10:22Z

one minor point of feedback: I noticed (in your google-drive-shared example of the old and new APIs) that when you deselect "old API" tag, the old-api-tagged elements disappear from the main body of the report (as expected) but their titles also disappear from the left-side TOC (not expected... I think I would want them to be disabled / greyed out maybe, rather than completely hidden?). This opinion is not super-strong, however, so I'm prepared to be talked out of it.

…p actions]

hoechenberger · 2021-09-08T19:17:47Z

@drammock Totally open for discussion on this one! We'd need to see how the scrollspy behaves if we do that the way you describe, though. But I would think that it's doable…

hoechenberger · 2021-09-12T20:55:04Z

mne/report/tests/test_report.py

    # should have down-up-down shape
    corr = np.corrcoef(norms, np.hanning(len(imgs)))[0, 1]
-    assert 0.78 < corr < 0.80
+    assert 0.778 < corr < 0.80


For passing locally on my Mac

hoechenberger · 2021-09-12T20:56:18Z

mne/viz/misc.py

+
+    Parameters
+    ----------
+    slices_as_figures : bool


I documented the new param I added, but didn't feel the urge to add docs for all the other params there were already undocumented before I came along 😅

drammock · 2021-09-13T15:35:43Z

mne/report/js_and_css/report.sass

+    position: relative;
+    padding-bottom: 5rem;
+}
+
+#content {
+    margin-top: 90px;
+    scroll-behavior: smooth;
+    position: relative;  // for scrollspy
+}
+
+#toc {
+    margin-top: 90px;
+    padding-bottom: 5rem;


sass does not use semicolons

Thank you, addressed in 3b1a73e

drammock · 2021-09-13T15:41:49Z

mne/report/report.py

+    from matplotlib import __version__ as MPL_VERSION
+    from pkg_resources import parse_version
+    if parse_version(MPL_VERSION) >= parse_version('3.2'):


elsewhere we mostly use from distutils.version import LooseVersion to do comparisons like this. Is this approach better / more modern?

distutils is being deprecated. +1 for adding a mne.fixes._version_greater(a, b, comp='>=') or something that uses pkg_resources if available and LooseVersion if not (comp could be '>=' or '>')

Great idea @larsoner, will give it a shot. The deprecation was actually the reason why I picked this approach, yes.

@larsoner Please take a look at f29eced and let me know what you think! :)

@larsoner FYI I made a followup commit to add some fixes, b6f4f5c

mne/fixes.py

larsoner · 2021-09-14T15:31:34Z

@hoechenberger let me know when I should look

hoechenberger · 2021-09-16T10:38:30Z

The only thing left on my todo list (top post) is "Update tutorial".

@larsoner I've tried to rework the git history a little by soft-resetting the branch to cb1916f (the last commit before I moved report.py to a new sub-directory) and committing all the changes done thereafter, hoping that this would preserve the history of the file's content. While this seems to be doing "the right thing" locally, GH doesn't appear to agree and still insists that report.py was deleted and a new file, report/report.py, was created from scratch.

If you have any advice / would like to give this a shot, please feel free to play with and force-push to this branch.

larsoner · 2021-09-16T16:59:07Z

Closing in favor of your next upcoming PR

hoechenberger added 2 commits September 7, 2021 21:01

WIP: Rework report

20c2dbe

Add icon

ec6da43

hoechenberger commented Sep 7, 2021

View reviewed changes

hoechenberger added 2 commits September 8, 2021 09:09

Cleanup

26da20b

Update Bootstrap to 5.1.1

c721d26

Should fix scrollspy issues

Some flake8 cleanups [ci skip]

cb1916f

sappelhoff reviewed Sep 8, 2021

View reviewed changes

hoechenberger added 6 commits September 8, 2021 10:32

Move to own sub-directory; remove jQuery-ui [ci skip]

e38e601

Move templates to individual files [ci skip]

05c0ce7

Cleanup [ci skip]

c5d7c9f

TOC template moved to separate file [ci skip]

fd11d59

No need for separate template for invididual TOC entries anymore

8d360d5

Remove unused occurrences of data_path [ci skip]

a8fa634

New method add_figs(); make add_figs_to_section() work again

250ebda

Fix add_bem_to_section(), fix for Circle, flake8 fixes [skip azp][ski…

987706b

…p actions]

Remove some cruft [circle full][skip azp][skip actions]

447aaac

hoechenberger commented Sep 12, 2021

View reviewed changes

Fix plot_bem()

e8715d4

drammock reviewed Sep 13, 2021

View reviewed changes

Add _compare_version() to fixes.py

f29eced

hoechenberger commented Sep 13, 2021

View reviewed changes

mne/fixes.py Outdated Show resolved Hide resolved

hoechenberger commented Sep 13, 2021

View reviewed changes

mne/fixes.py Outdated Show resolved Hide resolved

hoechenberger added 2 commits September 13, 2021 22:42

Fixes for fixes 🥸

b6f4f5c

Remove semicolons

3b1a73e

hoechenberger commented Sep 13, 2021

View reviewed changes

mne/fixes.py Outdated Show resolved Hide resolved

hoechenberger added 2 commits September 13, 2021 23:37

Add vertical scrollbar to tags dropdown

a715cf2

Fix typo

b8377a8

hoechenberger added 2 commits September 15, 2021 14:42

Add comprehensive test

63681d9

Add docstrings to test functioins

23b9ad2

hoechenberger force-pushed the report branch 3 times, most recently from f239514 to 23b9ad2 Compare September 15, 2021 17:07

hoechenberger added 6 commits September 15, 2021 22:14

Simplifications

4959bd7

Fix tests

93ae209

Make _iterate_files() a method

4e72f72

Fix test for systems without 3D backend

ee85cb3

flake

31842a4

Add test for content sorting

240e9e7

Fix remove() docstring

cc1c7b3

larsoner closed this Sep 16, 2021

hoechenberger mentioned this pull request Sep 16, 2021

Rework Reports (new history) #9754

Merged

Uh oh!

WIP: Rework report – Looking for feedback! #9722

WIP: Rework report – Looking for feedback! #9722

Uh oh!

Conversation

hoechenberger commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

What's new

Methods

"Data model"

UI

Dependencies, custom JavaScript & CSS

MWE

Issues & todo

Low prio / do later

Uh oh!

hoechenberger Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger commented Sep 7, 2021

Uh oh!

rob-luke commented Sep 7, 2021

Uh oh!

jasmainak commented Sep 8, 2021

Uh oh!

jasmainak commented Sep 8, 2021

Uh oh!

hoechenberger commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rob-luke commented Sep 8, 2021

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Sep 8, 2021

Uh oh!

hoechenberger commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Sep 8, 2021

Uh oh!

hoechenberger commented Sep 8, 2021

Uh oh!

larsoner commented Sep 8, 2021

Uh oh!

hoechenberger commented Sep 8, 2021

Uh oh!

drammock commented Sep 8, 2021

Uh oh!

hoechenberger commented Sep 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoechenberger Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsoner commented Sep 14, 2021

Uh oh!

hoechenberger commented Sep 16, 2021

hoechenberger commented Sep 7, 2021 •

edited

Loading

hoechenberger Sep 7, 2021 •

edited

Loading

hoechenberger commented Sep 8, 2021 •

edited

Loading

hoechenberger commented Sep 8, 2021 •

edited

Loading

hoechenberger Sep 12, 2021 •

edited

Loading