Skip to content
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

Merged
merged 10 commits into from
Feb 6, 2024

Conversation

nabilalibou
Copy link
Contributor

@nabilalibou nabilalibou commented Feb 5, 2024

Hello,

Some documentation enhancement discussed in #12401

Copy link

welcome bot commented Feb 5, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

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.

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:

https://output.circle-artifacts.com/output/job/1e925b38-f8f2-44c7-9c40-c2c1df117da7/artifacts/0/html/auto_tutorials/preprocessing/55_setting_eeg_reference.html#eeg-reference-and-source-modeling

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.

Copy link
Member

@larsoner larsoner left a 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

doc/changes/devel/12401.other.rst Outdated Show resolved Hide resolved
tutorials/preprocessing/55_setting_eeg_reference.py Outdated Show resolved Hide resolved
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.

approved pending a good-looking HTML render on CircleCI. 🤞🏻

@drammock
Copy link
Member

drammock commented Feb 6, 2024

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

@larsoner
Copy link
Member

larsoner commented Feb 6, 2024

@drammock GitHub UI allowed me to change the name, so I took care of that part!

@drammock drammock enabled auto-merge (squash) February 6, 2024 16:55
@drammock drammock merged commit 9f0dfef into mne-tools:main Feb 6, 2024
27 of 28 checks passed
Copy link

welcome bot commented Feb 6, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

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