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.

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

[DOC] Add documentation for setting montage order #12160

Merged
merged 9 commits into from
Nov 3, 2023
Merged

[DOC] Add documentation for setting montage order #12160

merged 9 commits into from
Nov 3, 2023

Conversation

withmywoessner
Copy link
Contributor

Reference issue

Example: Fixes #5619

What does this implement/fix?

Adds documentation that explains that the montage must be set after adding a reference.

Additional information

Should I also add a warning that runs inside the code itself after running set_montage?

@withmywoessner withmywoessner marked this pull request as draft November 1, 2023 13:00
@withmywoessner withmywoessner changed the title Add documentation for set montage order [WIP] [DOC] Add documentation for set montage order Nov 1, 2023
@withmywoessner withmywoessner changed the title [WIP] [DOC] Add documentation for set montage order [WIP] [DOC] Add documentation for setting montage order Nov 1, 2023
@larsoner
Copy link
Member

larsoner commented Nov 1, 2023

Should I also add a warning that runs inside the code itself after running set_montage?

You could logger.info something but I think documenting makes the most sense. Maybe add something to the Notes of add_reference_channels that says it should be run before set_montage?

@withmywoessner
Copy link
Contributor Author

Should I also add a warning that runs inside the code itself after running set_montage?

You could logger.info something but I think documenting makes the most sense. Maybe add something to the Notes of add_reference_channels that says it should be run before set_montage?

Okay, thank you @larsoner. Also, just for future reference, do you prefer all warnings to be contained in the same box? Or should each warning be in its own box like I did:
image

@larsoner
Copy link
Member

larsoner commented Nov 2, 2023

I think separate warnings makes sense

@withmywoessner withmywoessner marked this pull request as ready for review November 3, 2023 01:52
@withmywoessner withmywoessner changed the title [WIP] [DOC] Add documentation for setting montage order [DOC] Add documentation for setting montage order Nov 3, 2023
@withmywoessner
Copy link
Contributor Author

@larsoner Ready for merge!

larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
doc/changes/devel.rst Outdated Show resolved Hide resolved
larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
mne/_fiff/reference.py Outdated Show resolved Hide resolved
larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
mne/_fiff/reference.py Outdated Show resolved Hide resolved
larsoner
larsoner approved these changes Nov 3, 2023
View reviewed changes
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 some minor link improvements, I'll mark for merge-when-green, thanks @withmywoessner !

larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
mne/_fiff/meas_info.py Outdated Show resolved Hide resolved
@larsoner larsoner enabled auto-merge (squash) November 3, 2023 13:40
larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
doc/changes/devel.rst Outdated Show resolved Hide resolved
@larsoner larsoner merged commit fcb5926 into mne-tools:main Nov 3, 2023
28 checks passed
larsoner added a commit to larsoner/mne-python that referenced this pull request Nov 3, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into kernel
3611ef8 
* upstream/main: (35 commits)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  DOC: Fix typo found by codespell (mne-tools#12140)
  DOC: Document governance updates (mne-tools#12133)
  ...
larsoner added a commit to JD-Zhu/mne-python that referenced this pull request Nov 3, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into jdz
cf08258 
* upstream/main: (26 commits)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  ...
larsoner added a commit to pablomainar/mne-python that referenced this pull request Nov 8, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into fix
31c299b 
* upstream/main:
  BUG: Fix bug with spectrum warning (mne-tools#12186)
  Add argument splash to disable splash-screen from Qt-browser (mne-tools#12185)
  BUG: Fix bug with logging and n_jobs>1 (mne-tools#12154)
  Use gray logo (works in light and dark modes) (mne-tools#12184)
  Tweak logo for dark mode (mne-tools#12176)
  ENH: Improve Covariance.__repr__ (mne-tools#12181)
  ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405)
  Tweak README.rst (mne-tools#12166)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12177)
  MAINT: Add branch coverage (mne-tools#12174)
  OpenSSF (mne-tools#12175)
  fix docstring in 60_sleep.py (mne-tools#12171)
  FIX: skip empty lines in read_raw_eyelink (mne-tools#12172)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
@withmywoessner @snwnde
[DOC] Add documentation for setting montage order (mne-tools#12160)
b5aca2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner approved these changes

@drammock drammock Awaiting requested review from drammock drammock is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding a reference does not work after a montage has been set
2 participants
@withmywoessner @larsoner