Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 31, 2022

This small PR extracted from #10224

  • renames the MRI groupbox from "Transform" to "HEAD <> MRI Transform"
  • renames the info groupbox from "Digitization" to "Info source with digitization"
  • removes project_eeg from the view options (and only the view options, it's still possible to enable it from command line for example)

plus minor edits

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jan 31, 2022
@larsoner
Copy link
Member

Let's get rid of it from the command line, too. YAGNI and we get more - lines that way

@GuillaumeFavelier GuillaumeFavelier changed the title ENH: coreg UI WIP, ENH: coreg UI Jan 31, 2022
@GuillaumeFavelier
Copy link
Contributor Author

Let's get rid of it from the command line, too

Perfect! I was wondering actually

@GuillaumeFavelier
Copy link
Contributor Author

I ended up removing a lot of lines in the end.

@GuillaumeFavelier
Copy link
Contributor Author

What is the best way to deal with mne/tests/test_docstring_parameters.py with so many changes to the doc of coregistration()?

I do not have this behaviour with CoregistrationUI

@larsoner
Copy link
Member

The parameters in the signature should match the set in the docstring. If some are deprecated, you just change their description like:

project_eeg : bool
    Deprecated. Use :func:`mne.viz.plot_alignment` to see projected EEG electrodes.

@GuillaumeFavelier
Copy link
Contributor Author

My diff is really boring:

tabbed : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
split : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
scrollable : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
head_inside : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
guess_mri_subject : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
scale : float
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.
project_eeg : bool
	Deprecated. Use :func:`mne.viz.plot_alignment` to see projected EEG electrodes.
advanced_rendering : bool
	Deprecated. This parameter is not supported by the pyvistaqt 3d backend.

@hoechenberger
Copy link
Member

My diff is really boring:

I liked this one 😅

@GuillaumeFavelier
Copy link
Contributor Author

I decided to remove only project_eeg in here because I do not want to delay this further (it was supposed to be a small one 😅 ). We can always remove the rest in another PR.

@GuillaumeFavelier GuillaumeFavelier changed the title WIP, ENH: coreg UI ENH: coreg UI Feb 1, 2022
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 1, 2022

I do not understand why this fails (only!) on linux / conda / py3.9. I'll try locally on a new env.

=================================== FAILURES ===================================
______________________ test_coreg_gui_pyvista[pyvistaqt] _______________________
mne/gui/tests/test_coreg_gui.py:123: in test_coreg_gui_pyvista
    coregistration(**kwargs)
<decorator-gen-552>:12: in coregistration
    ???
mne/gui/__init__.py:194: in coregistration
    return CoregistrationUI(
<decorator-gen-565>:12: in __init__
    ???
mne/gui/_coreg.py:237: in __init__
    self.coreg = Coregistration(
mne/coreg.py:1371: in __init__
    self._setup_bem()
mne/coreg.py:1411: in _setup_bem
    raise RuntimeError("No standard head model was "
E   RuntimeError: No standard head model was found for subject

@GuillaumeFavelier GuillaumeFavelier marked this pull request as draft February 1, 2022 14:15
@hoechenberger
Copy link
Member

@GuillaumeFavelier I think I've seen this issue before locally after unsetting the SUBJECTS_DIR environment variable und passing subjects_dir=None

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 1, 2022

Do you have an idea on how to fix this? It does not make sense to me because CoregistrationUI should send the DeprecationWarning way before _setup_bem() is called but clearly there is something I missed.

@GuillaumeFavelier
Copy link
Contributor Author

And I just tried on a clean conda env, I cannot reproduce.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 1, 2022

Explaining it to you made me realize the issue @hoechenberger 😅

Indeed it's only a DeprecationWarning (not an error) so it does not interrupt the execution and it does continue into _setup_bem().

I'll push something to fix that 👍

@hoechenberger
Copy link
Member

Perfect! @GuillaumeFavelier I'll be home in 30 min and can test then. Also happy to make a screen share session if you want! Just ping me

@GuillaumeFavelier GuillaumeFavelier marked this pull request as ready for review February 1, 2022 16:15
@hoechenberger hoechenberger merged commit 623645a into mne-tools:main Feb 1, 2022
@hoechenberger
Copy link
Member

Thanks @GuillaumeFavelier!

@GuillaumeFavelier GuillaumeFavelier deleted the fix/coreg branch February 1, 2022 17:04
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.

3 participants