Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Oct 7, 2021

A smaller commit to isolate a potential problem.

Comment on lines -286 to -288
report = mne.Report()
report.add_sys_info(title='System info')
report.save('report_sys_info.html', overwrite=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this is problematic @hoechenberger but I'm inclined to remove it for now (merge this PR), then fix the bug in PyVista separately, then put this back in once it works properly. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's remove this to allow us to move on. The lost test coverage will only be minimal, it's just a convenience method anyway that calls another one that is tested separately.

Copy link
Member

Choose a reason for hiding this comment

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

And it will potentially save a few seconds of testing time

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's not a test, but the tutorial, sorry, I misread this. But yes it's okay to remove this. Please do what you think is necessary

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

green ! merge if happy @larsoner

@larsoner larsoner merged commit dd5ca5b into mne-tools:main Oct 7, 2021
@larsoner larsoner deleted the report2 branch October 7, 2021 20:11
@larsoner
Copy link
Member Author

larsoner commented Oct 7, 2021

Thanks @hoechenberger @agramfort @drammock for the quick feedback, hopefully we get a green Circle next!

larsoner added a commit to rob-luke/mne-python that referenced this pull request Oct 11, 2021
* upstream/main:
  MAINT: Update broken link, fix rendering (mne-tools#9829)
  Ensure plot_ica_sources() always plots traces of rejected ICs on top (mne-tools#9823)
  Improve plot_ica_sources() docstring (mne-tools#9825)
  MRG: Fix docstring for plot_ica_components() (mne-tools#9826)
  unpin jsonschema and filter its warning instead (mne-tools#9822)
  Add warning for SNIRF files with != 2 wavelengths (mne-tools#9817)
  add show_scalebars param to epochs.plot() (mne-tools#9815)
  MRG: Allow _plot_mri_contours() to return arrays (mne-tools#9818)
  MRG: Expand ~ in _check_fname() (mne-tools#9613)
  Improve ICA.plot_overlay() docstrings (mne-tools#9820)
  WIP, MAINT: Fix CircleCI (again) (mne-tools#9814)
  MRG, ENH: Add options to fit_dipole (mne-tools#9810)
  Rework Reports (new history) (mne-tools#9754)
  MRG, CI: Use VTK pre (mne-tools#9803)
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.

3 participants