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

Fix coreg tutorial typos #10716

Merged
merged 6 commits into from
Jun 7, 2022
Merged

Conversation

jhouck
Copy link
Contributor

@jhouck jhouck commented Jun 7, 2022

Reference issue

Minor cleanup, no issue

What does this implement/fix?

I noticed a few minor typos (e.g., slashes not quoted) and have addressed. Copy-and-paste into python should now work for this one. Also added a cite for the paper that assessed the reliability of the method.

@mmagnuski
Copy link
Member

The data_path / 'dirname' parts are not typos, we now use pathlib objects for paths management. Instead of op.join(...) you can simply use slash to join with system-specific separator.

automated MEG-MRI coregistration via scripting.
automated MEG-MRI coregistration via scripting. Generally the results of
this approach are consistent with those obtained from manual
coregistration :footcite:`HouckClaus2020`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
coregistration :footcite:`HouckClaus2020`
coregistration :footcite:`HouckClaus2020`.


data_path = mne.datasets.sample.data_path()
subjects_dir = data_path + '/subjects'
data_path = Path(mne.datasets.sample.data_path())
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already a Path object, so no need to import and convert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an attempt to make the use of the Path object more obvious. Maybe a comment instead of the redundant import/conversion?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is documented. If you think a comment is necessary to ahead and add it.

@jhouck
Copy link
Contributor Author

jhouck commented Jun 7, 2022

@mmagnuski I knew that, and still I did this. I've removed the errors that I'd introduced and added an explicit call to pathlib.Path along with @cbrnr's catch.

Also, the tutorial mentions mne.scale_mri but I don't see an example or tutorial elsewhere that uses it. Is there one that I'm missing? That might be beyond the scope of this PR. The previous guide was in https://www.slideshare.net/mne-python but things have changed a bit since then.

@mmagnuski
Copy link
Member

I don't think we have an example showing the usage of mne.scale_mri. Might be useful to add in the future.

@larsoner larsoner merged commit f176d91 into mne-tools:main Jun 7, 2022
@larsoner
Copy link
Member

larsoner commented Jun 7, 2022

Thanks @jhouck !

@jhouck jhouck deleted the Fix_coreg_tutorial_typos branch June 7, 2022 23:26
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 8, 2022
* upstream/main:
  MRG: In Report, allow to add multiple figure or image elements to the same content block ("section") (mne-tools#10694)
  CI: Add style conditional (mne-tools#10723)
  Fix coreg tutorial typos (mne-tools#10716)
  MAINT: Deal with nibabel deprecation (mne-tools#10719)
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.

4 participants