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
nabilalibou
requested review from
agramfort,
dengemann and
drammock
as code owners
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
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
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Just two suggested tweaks to make the RST render more nicely, otherwise LGTM
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
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 <dan@mccloy.info> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
3 participants
Hello,
Some documentation enhancement discussed in #12401