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

[MAINT, MRG] Refactor BEM code #9625

Merged
merged 3 commits into from
Jul 29, 2021
Merged

[MAINT, MRG] Refactor BEM code #9625

merged 3 commits into from
Jul 29, 2021

Conversation

alexrockhill
Copy link
Contributor

This wasn't nearly as much of a pain as I thought. This simplifies things quite a bit and has the code from developing the GUI that can now be shared in _freesurfer.py.

Addresses comments in #9586.

Basically, instead of this very confusing which axis to flip and order of everything, the MRI is just reoriented to RAS and everything is follows from there.

Should be easy to use this code in the GUI, good idea @larsoner.

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.

Awesome, glad to see that it was so simple!

Since the BEM and GUI code also share a lot of common viz stuff (labeling, surface contours, slice numbers, etc.) hopefully we can continue to refactor that stuff as we go in subsequent PRs. To me the tasks of:

  1. look at where this BEM boundary is on the MRI
  2. look at where this dipole is in the MRI, and
  3. display MRI slices so I can click on them

are all shared EDIT: orthorview-like tasks to it would be good to continue to refactor them. IIRC we unified the dipole plotting code with the BEM plotting code years ago and both benefitted from it, so I'm hoping similar advantages can come from the electrode GUI work!

Coming back to this PR, can you touch some examples that use plot_dipole_locations, Report, and plot_bem? That way we can visually verify that the output hasn't changed. IIRC there should be unit tests that yell at you if something is wrong with the plotting, but who knows. I'll see if I can dig up some old issues that led to refactoring before to see if we can visually verify they still work okay...

mne/viz/misc.py Show resolved Hide resolved
mne/_freesurfer.py Show resolved Hide resolved
@larsoner
Copy link
Member

... also I might be wrong about BEM and dipoles being shared. Looking a bit, that code uses _plot_dipole_mri_orthoview. I didn't follow the trail the whole way, though. Maybe there is some more refactoring we can do at some point. But for now I think continuing to refactor BEM + GUI to simplify / enhance both is good!

@alexrockhill
Copy link
Contributor Author

alexrockhill commented Jul 28, 2021

... also I might be wrong about BEM and dipoles being shared. Looking a bit, that code uses _plot_dipole_mri_orthoview. I didn't follow the trail the whole way, though. Maybe there is some more refactoring we can do at some point. But for now I think continuing to refactor BEM + GUI to simplify / enhance both is good!

Hmmm yeah I was going to say, _plot_mri_contours was only used in report. I'll touch the examples that use Report and plot_bem though.

@larsoner
Copy link
Member

Okay just a commit that touches Report and plot_bem examples would be good, then. Alternatively you can run them locally and verify they haven't changed and report back

@alexrockhill
Copy link
Contributor Author

alexrockhill commented Jul 28, 2021

Okay just a commit that touches Report and plot_bem examples would be good, then. Alternatively you can run them locally and verify they haven't changed and report back

Reporting back, they are unchanged but I touched them anyway.

@larsoner
Copy link
Member

Ideally we would have some non-standard-orientation MRI to test with, but given that even standard FreeSurfer axes are not ordered RAS and this code generalizes everything, I think we'll be okay. I'll merge once CIs come back happy!

@alexrockhill
Copy link
Contributor Author

Ok good to go!

@larsoner larsoner merged commit b66734c into mne-tools:main Jul 29, 2021
@larsoner
Copy link
Member

Thanks @alexrockhill !

@alexrockhill alexrockhill deleted the bem branch July 29, 2021 17:02
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.

2 participants