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

[MRG] Improve docstring format and support floats for conductivity in make_bem_model #12020

Merged
merged 40 commits into from
Sep 30, 2023

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne marked this pull request as draft September 27, 2023 14:53
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.

oops, didn't notice this was in draft mode when I started reviewing. Stopping mid-way. Ping when ready for a complete review

doc/_includes/bem_model.rst Outdated Show resolved Hide resolved
doc/_includes/channel_interpolation.rst Outdated Show resolved Hide resolved
doc/_includes/forward.rst Outdated Show resolved Hide resolved
mne/cov.py Outdated Show resolved Hide resolved
mne/cov.py Outdated Show resolved Hide resolved
mne/cov.py Outdated Show resolved Hide resolved
mne/cov.py Outdated Show resolved Hide resolved
mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@mscheltienne
Copy link
Member Author

The cookbook! I can't believe it took me so long to discover this lovely page! A grep on cookbook doesn't yield any match outside its definition, and that's a pity. WDYT about moving it to the top of the Documentation toctree and to include a ref in the introductory tutorial "Overview of MEG/EEG analysis with MNE-Python"?

Screenshot from 2023-09-27 17-30-26

To my knowledge (and I've been digging around during a good part of my afternoon), that's the only document which chains all the operation needed to get from a raw to an stc. Until now, I rarely looked at sources, and it took me a bit too long and a fair share of frustration to get there..

@drammock
Copy link
Member

The cookbook! I can't believe it took me so long to discover this lovely page!

see #6778

Copy link
Member Author

@mscheltienne mscheltienne left a comment

Choose a reason for hiding this comment

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

This PR adds x-ref and style fixes to docstrings + the 3 code changes listed below.

Comment on lines -685 to +695
conductivity = np.array(conductivity, float)
conductivity = np.atleast_1d(conductivity).astype(float)
if conductivity.ndim != 1 or conductivity.size not in (1, 3):
raise ValueError("conductivity must be 1D array-like with 1 or 3 " "elements")
raise ValueError(
"conductivity must be a float or a 1D array-like with 1 or 3 elements"
)
Copy link
Member Author

Choose a reason for hiding this comment

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

First code-change in this PR, supporting floats directly for conductivity instead of array of shape (1, ) for single-layer model.

mne/cov.py Outdated Show resolved Hide resolved
mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved
Comment on lines 1656 to 1657
n_jobs=None,
*,
Copy link
Member Author

Choose a reason for hiding this comment

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

Second code change, add the argument n_jobs to setup_volume_source_space used by _make_volume_source_space.

sharex=True,
sharey=False,
figsize=(8.8, 2.2 * n_rows),
constrained_layout=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.

Third code change: use a contrained_layout to fix this kind of plot:

Screenshot from 2023-09-28 12-51-19

@mscheltienne mscheltienne marked this pull request as ready for review September 28, 2023 10:53
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 comments, otherwise looks great!

mne/minimum_norm/inverse.py Outdated Show resolved Hide resolved
mne/source_space/_source_space.py Outdated Show resolved Hide resolved
mne/source_space/_source_space.py Show resolved Hide resolved
mne/viz/evoked.py Outdated Show resolved Hide resolved
@mscheltienne mscheltienne changed the title Improve docstring format and support floats for conductivity in make_bem_model [MRG] Improve docstring format and support floats for conductivity in make_bem_model Sep 29, 2023
@larsoner
Copy link
Member

@mscheltienne don't forget to ping us after you push commits assuming you're done, I have turned off commit notifications in my GitHub preferences (and I assume many other maintainers have as well)! I should be able to look and merge tomorrow

@mscheltienne
Copy link
Member Author

Sure, no hurry on that one ;)

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.

@larsoner larsoner merged commit e99ea74 into mne-tools:main Sep 30, 2023
26 checks passed
@larsoner larsoner added the ENH label Oct 2, 2023
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
… make_bem_model (mne-tools#12020)

Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants