Skip to content

ENH: More complete report repr #11357

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.

Sign up for GitHub

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

Jump to bottom
Merged
larsoner merged 3 commits into from
Dec 7, 2022
Merged

ENH: More complete report repr #11357

larsoner merged 3 commits into from
Dec 7, 2022

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Dec 7, 2022

It's useful to:

  1. Know the number of titles in addition to content items (each title/section can have multiple content items)
  2. Have all titles listed
  3. Know their sizes

Changes the report repr for ds000248_base in MNE-BIDS-Pipeline from:

$ python -c "import mne; print(mne.report.open_report('sub-01_task-audiovisual_report.h5'))"
<Report | 79 items | sub-01, task-audiovisual
Info
Time series
 ...
Configuration file
System information
>

to

$ python -c "import mne; print(mne.report.open_report('sub-01_task-audiovisual_report.h5'))"
<Report | 29 titles | 79 items | sub-01, task-audiovisual  | 60.3 MB
 Raw (original run 01), run 01                             | 1.8 MB
 Data quality                                              | 0.5 MB
 Raw (maxwell filtered), run 01                            | 1.7 MB
 Raw (filtered), run 01                                    | 1.8 MB
 Events                                                    | 0.0 MB
 Epochs: before cleaning                                   | 0.8 MB
 Epochs: after cleaning                                    | 0.8 MB
 Condition: Auditory                                       | 2.5 MB
 Condition: Visual                                         | 2.6 MB
 Condition: Auditory/Left                                  | 2.4 MB
 Condition: Auditory/Right                                 | 2.7 MB
 Contrast: Visual+Auditory                                 | 2.8 MB
 Contrast: Auditory/Right+Auditory/Left                    | 2.8 MB
 Decoding: full-epochs                                     | 0.0 MB
 Decoding: time-by-time                                    | 0.1 MB
 TFR Power: Auditory                                       | 0.0 MB
 TFR ITC: Auditory                                         | 0.2 MB
 TFR Power: Visual                                         | 0.0 MB
 TFR ITC: Visual                                           | 0.2 MB
 Noise covariance                                          | 1.6 MB
 BEM                                                       | 4.3 MB
 Sensor alignment                                          | 0.6 MB
 Forward solution                                          | 0.0 MB
 Source: Auditory                                          | 7.5 MB
 Source: Visual                                            | 7.6 MB
 Source: Auditory/Left                                     | 7.5 MB
 Source: Auditory/Right                                    | 7.5 MB
 Configuration file                                        | 0.0 MB
 System information                                        | 0.0 MB
>

drammock
drammock approved these changes Dec 7, 2022
View reviewed changes
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.

LGTM! Bonus points if you align the sizes on the decimal point :) The total size decimal doesn't align with the section sizes.

@cbrnr
Copy link
Contributor

cbrnr commented Dec 7, 2022

In my opinion, __repr__() should not be used like that. I know you probably want to avoid the additional print(), but __repr__() should represent the object and reflect how it was created.

@larsoner
Copy link
Member Author

larsoner commented Dec 7, 2022

I know you probably want to avoid the additional print(), but repr() should represent the object and reflect how it was created.

I think the existing repr already violates this, and we don't generally follow this "bare repr" practice in MNE. Epochs for example documents its conditions and trial counts (and size in memory), Raw documents its filename and size in memory, info documents all its current contents (regardless of how it was created initially) in human-readable form, etc. So it's less about creation and more about reflecting its current state in some helpful way. So to me this PR just fleshes the "current state" out a bit more in the way that's consistent with our current practice.

If we want to make the reprs more bare, I think it's worth having a more complete separate discussion...

@cbrnr
Copy link
Contributor

cbrnr commented Dec 7, 2022

I agree, most of our reprs do not follow this convention, but none of them is so extensive. And yes, whether or not we should compress our reprs in general should be a separate condition. I just wanted to point it out here, but please feel free to ignore for now and merge (I see the advantage of having the most relevant information there of course).

@larsoner
Copy link
Member Author

larsoner commented Dec 7, 2022

Bonus points if you align the sizes on the decimal point :) The total size decimal doesn't align with the section sizes.

Done

<Report | 29 titles | 79 items | sub-01, task-audiovisual  | 60.3 MB
 Raw (original run 01), run 01                             |  1.8 MB
 Data quality                                              |  0.5 MB
 Raw (maxwell filtered), run 01                            |  1.7 MB
 Raw (filtered), run 01                                    |  1.8 MB
 Events                                                    |  0.0 MB
 Epochs: before cleaning                                   |  0.8 MB
 Epochs: after cleaning                                    |  0.8 MB
 Condition: Auditory                                       |  2.5 MB
 Condition: Visual                                         |  2.6 MB
 Condition: Auditory/Left                                  |  2.4 MB
 Condition: Auditory/Right                                 |  2.7 MB
 Contrast: Visual+Auditory                                 |  2.8 MB
 Contrast: Auditory/Right+Auditory/Left                    |  2.8 MB
 Decoding: full-epochs                                     |  0.0 MB
 Decoding: time-by-time                                    |  0.1 MB
 TFR Power: Auditory                                       |  0.0 MB
 TFR ITC: Auditory                                         |  0.2 MB
 TFR Power: Visual                                         |  0.0 MB
 TFR ITC: Visual                                           |  0.2 MB
 Noise covariance                                          |  1.6 MB
 BEM                                                       |  4.3 MB
 Sensor alignment                                          |  0.6 MB
 Forward solution                                          |  0.0 MB
 Source: Auditory                                          |  7.5 MB
 Source: Visual                                            |  7.6 MB
 Source: Auditory/Left                                     |  7.5 MB
 Source: Auditory/Right                                    |  7.5 MB
 Configuration file                                        |  0.0 MB
 System information                                        |  0.0 MB
>

I agree, most of our reprs do not follow this convention, but none of them is so extensive.

FWIW I've seen some pretty long infos that at least come close to the length above. For example, here's an anonymized info at 25 lines (the report above is 31):

>>> mne.io.read_raw_fif('test_move_anon_raw.fif', allow_maxshield='yes').info
<Info | 23 non-empty values
 acq_pars: ACQch001 110113 ACQch002 110112 ACQch003 110111 ACQch004 110122 ...
 bads: []
 ch_names: MEG0113, MEG0112, MEG0111, MEG0122, MEG0123, MEG0121, MEG0132, ...
 chs: 204 Gradiometers, 102 Magnetometers, 11 Stimulus, 60 EEG, 2 EOG, 1 ECG, 12 misc
 custom_ref_applied: False
 description: Vectorview system
 dev_head_t: MEG device -> head transform
 dig: 193 items (3 Cardinal, 5 HPI, 61 EEG, 124 Extra)
 experimenter: neuromag
 file_id: 4 items (dict)
 highpass: 0.0 Hz
 hpi_meas: 1 item (list)
 hpi_results: 1 item (list)
 hpi_subsystem: 3 items (dict)
 line_freq: 60.0
 lowpass: 326.4 Hz
 maxshield: True
 meas_date: 2015-04-20 21:38:28 UTC
 meas_id: 4 items (dict)
 nchan: 392
 proc_history: 1 item (list)
 proj_id: 1 item (ndarray)
 proj_name: eric_non_space
 projs: mag.fif : PCA-v1: off, mag.fif : PCA-v2: off, mag.fif : PCA-v3: ...
 sfreq: 1200.0 Hz
>

@cbrnr
Copy link
Contributor

cbrnr commented Dec 7, 2022

FWIW I've seen some pretty long infos that at least come close to the length above. For example, here's an anonymized info at 25 lines (the report above is 31):

Yep, that's also too long (and I know I was the one who extended it 😄).

@drammock drammock enabled auto-merge (squash) December 7, 2022 16:14
@larsoner larsoner disabled auto-merge December 7, 2022 17:46
@larsoner larsoner merged commit 1be103d into mne-tools:main Dec 7, 2022
@larsoner larsoner deleted the report branch December 7, 2022 17:46
larsoner added a commit to larsoner/mne-python that referenced this pull request Dec 7, 2022
@larsoner
ENH: More complete report repr (mne-tools#11357)
f562265
larsoner added a commit to larsoner/mne-python that referenced this pull request Dec 7, 2022
@larsoner
ENH: More complete report repr (mne-tools#11357)
0211fd5
larsoner added a commit to britta-wstnr/mne-python that referenced this pull request Dec 8, 2022
@larsoner
Merge remote-tracking branch 'upstream/main' into add_fwhm_morlet
c689886 
* upstream/main:
  ENH: Add webp support to Report (mne-tools#11359)
  ENH: More complete report repr (mne-tools#11357)
  MAINT: Simplify server installation instructions (mne-tools#11356)
  BUG: Fix where report replacement did not respect section (mne-tools#11346)
  [DOC] Fix video link for coregistration (mne-tools#11354)
larsoner added a commit to ealtamir/mne-python that referenced this pull request Dec 8, 2022
@larsoner
Merge remote-tracking branch 'upstream/main' into patch-1
5d2af8f 
* upstream/main:
  ENH: Add webp support to Report (mne-tools#11359)
  ENH: More complete report repr (mne-tools#11357)
  MAINT: Simplify server installation instructions (mne-tools#11356)
  BUG: Fix where report replacement did not respect section (mne-tools#11346)
  [DOC] Fix video link for coregistration (mne-tools#11354)
  Revert "[ENH] Add tutorial on time-frequency source estimation with STC viewer GUI" (mne-tools#11350)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drammock drammock drammock approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@larsoner @cbrnr @drammock