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] Adding updated tutorial to mention BIDS vs mne coordinate systems. #717

Merged
merged 16 commits into from
Mar 12, 2021

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Feb 28, 2021

PR Description

Closes: #715

To work on after #704 is merged.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@hoechenberger
Copy link
Member

Awesome, @adam2392! Ping me when you want some feedback

@adam2392
Copy link
Member Author

https://4136-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_ieeg_to_bids.html#sphx-glr-auto-examples-convert-ieeg-to-bids-py

how's this for first pass?

Once CI passes, then it's just however we want to describe these issues in this tutorial.

@adam2392
Copy link
Member Author

adam2392 commented Mar 3, 2021

@hoechenberger any issues w/ this?

@hoechenberger
Copy link
Member

hoechenberger commented Mar 3, 2021

There's a rendering issue:

Screen Shot 2021-03-03 at 19 59 28

And you're sometimes talking about "MNE", sometimes about "MNE-Python"; I think this can be made more consistent :)

Also I think this important info is somehow getting lost, being buried down there at the very end of the tutorial. Can we maybe move it up somehow? I think it deserves its own Info Box or something…

@adam2392
Copy link
Member Author

adam2392 commented Mar 3, 2021

There's a rendering issue:

Screen Shot 2021-03-03 at 19 59 28

Why are my sphinx/rst skills so subpar :(.

And you're sometimes talking about "MNE", sometimes about "MNE-Python"; I think this can be made more consistent :)

Also I think this important info is somehow getting lost, being buried down there at the very end of the tutorial. Can we maybe move it up somehow? I think it deserves its own Info Box or something…

agreed. Will adjust

@hoechenberger
Copy link
Member

Why are my sphinx/rst skills so subpar :(.

Because RST is a horrible standard and Sphinx syntax is just as horrible too 🤯 My personal opinion ;)

@codecov-io
Copy link

Codecov Report

Merging #717 (27af1bb) into master (a45695e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #717   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files          23       23           
  Lines        2759     2759           
=======================================
  Hits         2591     2591           
  Misses        168      168           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a45695e...29cef07. Read the comment docs.

@adam2392 adam2392 mentioned this pull request Mar 12, 2021
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

@adam2392 I think this should work.

examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved
examples/convert_ieeg_to_bids.py Outdated Show resolved Hide resolved
@sappelhoff
Copy link
Member

Don't forget to add this to the bottom of the file after merging my two suggestions.

.. _appendix VIII: https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html
.. _background on FreeSurfer: https://mne.tools/dev/auto_tutorials/source-modeling/plot_background_freesurfer_mne
.. _MNE-Python coordinate frames: https://mne.tools/dev/auto_tutorials/source-modeling/plot_source_alignment.html

@sappelhoff
Copy link
Member

sometimes it's better to commit, then to comment and ask somebody to commit something :-D

@hoechenberger hoechenberger added this to the 0.7 milestone Mar 12, 2021
@adam2392
Copy link
Member Author

sometimes it's better to commit, then to comment and ask somebody to commit something :-D

Thanks!

@adam2392 adam2392 changed the title [WIP] Adding updated tutorial to mention BIDS vs mne coordinate systems. [MRG] Adding updated tutorial to mention BIDS vs mne coordinate systems. Mar 12, 2021
doc/whats_new.rst Outdated Show resolved Hide resolved
@sappelhoff sappelhoff merged commit 5136507 into mne-tools:master Mar 12, 2021
@adam2392 adam2392 deleted the tut branch March 15, 2021 02:36
@adam2392
Copy link
Member Author

Is there a reason it's not showing up in our renderings: https://mne.tools/mne-bids/dev/auto_examples/convert_ieeg_to_bids.html#

sappelhoff added a commit that referenced this pull request Mar 15, 2021
@sappelhoff
Copy link
Member

Is there a reason it's not showing up in our renderings: https://mne.tools/mne-bids/dev/auto_examples/convert_ieeg_to_bids.html#

fixed in df53001

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.

Better documentation for iEEG coordinate system/frames in examples
4 participants