Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

MRG, ENH: make plot alignment use defaults for colors #8553

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

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

agramfort
Copy link
Member

plot_alignment does not use the colors used in coreg. It should be consistent. This is just an example that leads to something like this:

figure_1

as opposed to master:

Figure_2

FYI this is obtained with this script:

import numpy as np
import matplotlib.pyplot as plt
import mne
from mne.datasets import sample
from mne.viz import set_3d_backend
from mne.viz._brain.view import views_dicts

set_3d_backend("pyvista")
data_path = sample.data_path()
raw_fname = data_path + '/MEG/sample/sample_audvis_raw.fif'
subjects_dir = data_path + '/subjects'
subject = 'sample'
trans = data_path + '/MEG/sample/sample_audvis_raw-trans.fif'
info = mne.io.read_info(raw_fname)
fig = mne.viz.plot_alignment(info, trans, subject=subject, dig=True,
                             meg=[],
                             # meg=['helmet', 'sensors'],
                             coord_frame='mri',
                             subjects_dir=subjects_dir,
                             surfaces=['head-dense'])


dists = mne.dig_mri_distances(info, trans, subject,
                              subjects_dir=subjects_dir)

images = []

views = ['frontal', 'lateral', 'medial']
views += ['axial', 'rostral', 'coronal']

for view in views:
    mne.viz.set_3d_view(fig, **views_dicts['both'][view])
    images.append(fig.plotter.screenshot())

images = np.concatenate(
    [np.concatenate(images[:3], axis=1),
     np.concatenate(images[3:], axis=1)],
    axis=0)

plt.title(f'Distance from {len(dists)} digitized points to head: '
          f'{1e3 * np.mean(dists):.2f} mm')
plt.imshow(images)
plt.axis('off')
plt.show()

thanks @GuillaumeFavelier for the help and the Neurospin group @virvw @SophieHerbst

this should be used to produce our automatic report on coregistration.

thoughts @larsoner @drammock @jasmainak @SherazKhan ?

@virvw
Copy link

virvw commented Nov 22, 2020

@agramfort use transparent skin but filled colors for fiducials and digitized points so we get a sense of depth

@virvw
Copy link

virvw commented Nov 22, 2020

add centroid for the nasion and for the ear fiducials as those are typically recorded with good precision by the researchers

@larsoner
Copy link
Member

add centroid for the nasion and for the ear fiducials as those are typically recorded with good precision by the researchers

I think these were originally large because @christianbrodbeck had the opposite sense, that there was some ambiguity in it (from a digitizer accuracy standpoint, probably). But I agree that with well behaved subjects you can more precise than we show here, and it shouldn't hurt to make them the same size as the MRI ones. I'd rather just shrink the markers rather than make a large translucent one and a small opaque one.

use transparent skin

We tried this default in #8446 and decided against it. So I think if people want this they should use the new dict support for surfaces by doing something like surfaces=dict(head=0.7).

filled colors for fiducials

Part of the reason the fids are translucent is because in theory the bem/subject-fiducials.fif that you mark in MRI space should align perfectly / overlap with the fiducials recorded using the digitizer, so we need some way to see two sets of points in the same space. This is also why the MRI fids are opaque and the digitizer fids are translucent (and much larger, but I agree this isn't really necessary, as above). One option would be to make the MRI fiducials some other shape like a square, and the digitized ones stay a circle. That way you can see both even if they line up. (I think it's okay to use squares even though they have sidedness / orientation -- we can align the sides with the XYZ/RAS of the MRI, so actually get a bit more information out of it.)

@christianbrodbeck
Copy link
Member

If I recall correctly the reason for making the digitizer fiducials large was mainly to be able to see them together with the MRI fiducials, and to see the even when, for example, they're sunk slightly under the MRI head surface. With template brains the expected alignment is less perfect.

@drammock
Copy link
Member

+1 for being consistent, and I think the yellowish head is more pleasant / easier to see than the gray one. Regarding fiducial size, etc, I think making them somewhat smaller would be OK, and I like the idea of MRI fids and digitizer fids being different geometric solids... actually I think a cube for one and a regular octahedron for the other would be ideal --- if you pick the relative sizes right it would make it really easy to tell if they line up:

Screenshot_2020-11-22_12-35-57

@agramfort
Copy link
Member Author

@drammock do you want to lead this following my gist above?

@larsoner
Copy link
Member

larsoner commented Nov 23, 2020

+1 for cube plus something else. I'm not sure VTK has an octahedron glyph, if not then cube+sphere seems fine (cube for MRI, sphere for dig) and can also have a nice clean overlap if their volumes are identical. This is also consistent with how dig points show up as spheres anyway...

I would start by trying to make them equal-volume, leading to a cube-side-to-sphere-radius ratio of:

>>> (3 / (4 * pi)) ** (1/3)
0.6203504908994001

This way perfect alignment means some parts of the sphere will protrude, and the corners of the cube will, too.

@drammock
Copy link
Member

@drammock do you want to lead this following my gist above?

how urgent is this? it might take me some time to get up to speed on vtk etc.

@larsoner
Copy link
Member

Not that urgent, but I think I can probably do it quickly -- let me take a look

@larsoner
Copy link
Member

Okay I took a try with the cube. I agree octahedron would be better but it will be a pain to implement.

$ mne coreg -d subjects -s sample --fif MEG/sample/sample_audvis_raw.fif --trans MEG/sample/sample_audvis_raw-trans.fif

Gives:

Screenshot from 2020-11-23 16-15-31

Screenshot from 2020-11-23 16-15-38

And with "fit fid" you get a different alignment where the nasion is aligned, you can see they are on top of each other at least:

And tutorials/source-modeling/plot_source_alignment.py gives:

Screenshot from 2020-11-23 16-12-27

@larsoner
Copy link
Member

larsoner commented Dec 9, 2020

Okay got octahedron working, ready for review/merge from my end:

Screenshot from 2020-12-09 11-46-39

Screenshot from 2020-12-09 11-45-56

@larsoner larsoner added this to the 0.22 milestone Dec 9, 2020
@larsoner larsoner changed the title WIP : make plot alignment use defaults for colors MRG, ENH: make plot alignment use defaults for colors Dec 9, 2020
Copy link
Member Author

@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.

@agramfort agramfort merged commit 9139c34 into mne-tools:master Dec 9, 2020
@agramfort
Copy link
Member Author

thx @larsoner !

@larsoner larsoner deleted the plot_align_colors branch December 9, 2020 20:13
@larsoner larsoner restored the plot_align_colors branch December 9, 2020 20:13
larsoner added a commit to agramfort/mne-python that referenced this pull request Dec 16, 2020
* upstream/master: (42 commits)
  MRG, ENH: Add DICS bias tests (mne-tools#8610)
  MRG, BUG, ENH: Add window option (mne-tools#8662)
  BUG: Fix alpha for volumes (mne-tools#8663)
  MRG, BUG: Fix bugs with envcorr (mne-tools#8658)
  MRG, ENH: Progressbar for csd_morlet (mne-tools#8608)
  Render is necessary now (mne-tools#8657)
  VIZ: Fix head size (mne-tools#8651)
  MRG, MAINT: bump sphinxcontrib-bitex version (mne-tools#8653)
  MRG, MAINT: Improve server env (mne-tools#8656)
  BUG: Mayavi center (mne-tools#8644)
  VIZ, ENH: allow show/hide annotations by label (mne-tools#8624)
  Add regression test for EEGLAB data with a chanlocs struct (mne-tools#8647)
  FIX: scalar_bar (mne-tools#8643)
  MRG: Small fix to tutorial; rename plot_events ordinate label to "Event id"; improve some SSP docstrings (mne-tools#8612)
  MRG, ENH: make plot alignment use defaults for colors (mne-tools#8553)
  BUG: Fix passing of channel type (mne-tools#8638)
  FIX: fixed loop over norm PSF/CTF options (mne-tools#8636)
  MRG, BUG: Pass kwargs (mne-tools#8630)
  DOC: Clearer error message (mne-tools#8631)
  BUG: Fix number of labels (mne-tools#8629)
  ...
larsoner added a commit to wmvanvliet/mne-python that referenced this pull request Dec 16, 2020
* upstream/master: (38 commits)
  MRG, ENH: Add DICS bias tests (mne-tools#8610)
  MRG, BUG, ENH: Add window option (mne-tools#8662)
  BUG: Fix alpha for volumes (mne-tools#8663)
  MRG, BUG: Fix bugs with envcorr (mne-tools#8658)
  MRG, ENH: Progressbar for csd_morlet (mne-tools#8608)
  Render is necessary now (mne-tools#8657)
  VIZ: Fix head size (mne-tools#8651)
  MRG, MAINT: bump sphinxcontrib-bitex version (mne-tools#8653)
  MRG, MAINT: Improve server env (mne-tools#8656)
  BUG: Mayavi center (mne-tools#8644)
  VIZ, ENH: allow show/hide annotations by label (mne-tools#8624)
  Add regression test for EEGLAB data with a chanlocs struct (mne-tools#8647)
  FIX: scalar_bar (mne-tools#8643)
  MRG: Small fix to tutorial; rename plot_events ordinate label to "Event id"; improve some SSP docstrings (mne-tools#8612)
  MRG, ENH: make plot alignment use defaults for colors (mne-tools#8553)
  BUG: Fix passing of channel type (mne-tools#8638)
  FIX: fixed loop over norm PSF/CTF options (mne-tools#8636)
  MRG, BUG: Pass kwargs (mne-tools#8630)
  DOC: Clearer error message (mne-tools#8631)
  BUG: Fix number of labels (mne-tools#8629)
  ...
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.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.