-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
DOC: Point out that inverse modeling needs an average reference projector ready to not raise an error #12420
Conversation
…ctor ready to not raise an error
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text changes look good, but the rendering of the final paragraph is messed up (it seems that italicizing the whole paragraph prevents the preformatting and cross-references from being handled). See here:
One fix is to just remove the *
at beginning and end that italicizes the whole paragraph. Another option is to convert the whole paragraph into an admonition. I kinda like the admonition approach; they were designed to call attention to things, and do a better job of that than just italics. It would be something like this:
# .. important::
# For the above reasons, when performing inverse imaging, MNE-Python will raise
# a ``ValueError`` if there are EEG channels present and something other than
# an average reference projector strategy has been specified. To ensure
# correct functioning consider calling
# :meth:`~mne.io.Raw.set_eeg_reference(projection=True)` to add an average
# reference projector*.
Final note: you either need to add a changelog entry (doc/changes/devel/12420.bugfix.rst
, see other files in that folder for format/inspiration), or add a PR label no-changelog-entry-needed
.
- Rephrase changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two suggested tweaks to make the RST render more nicely, otherwise LGTM
Co-authored-by: Eric Larson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved pending a good-looking HTML render on CircleCI. 🤞🏻
oops, one more fix needed: the changelog filename should have this PR number (12420) not the number of the issue it fixes (12401). See https://github.com/mne-tools/mne-python/actions/runs/7803187571/job/21282378841?pr=12420#step:3:16 |
@drammock GitHub UI allowed me to change the name, so I took care of that part! |
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
…ctor ready to not raise an error (mne-tools#12420) Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: Eric Larson <[email protected]>
Hello,
Some documentation enhancement discussed in #12401