-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Refactor test_plot_alignment_basic() #13472
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
base: main
Are you sure you want to change the base?
Conversation
drammock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! a few suggestions below (terse, because @nordme and I were reviewing this together in real time)
| def test_plot_alignment_cframe(renderer): | ||
| """Test varying the coordinate frame for alignment plot.""" | ||
| info = read_info(evoked_fname) | ||
| for coord_frame in ("meg", "head", "mri"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parametrize this
| surfaces="white", | ||
| coord_frame="head", | ||
| ) | ||
| fwd = convert_forward_solution(fwd, force_fixed=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment before this line, e.g. "now test a fwd with fixed source ori"
mne/viz/tests/test_3d.py
Outdated
| surfaces="white", | ||
| coord_frame="head", | ||
| ) | ||
| fwd["coord_frame"] = FIFF.FIFFV_COORD_MRI # check required to get to MRI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better comment (e.g. "hack fwd coordframe, to make sure plot_alignment raises error")
mne/viz/tests/test_3d.py
Outdated
| @testing.requires_testing_data | ||
| def test_plot_alignment_fwd(renderer): | ||
| """Test plotting forward solution.""" | ||
| info = read_info(evoked_fname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace with info=evoked.info and use evoked fixture
mne/viz/tests/test_3d.py
Outdated
| sample_vsrc.plot(subjects_dir=subjects_dir, head=True, skull=True, brain="white") | ||
| renderer.backend._close_all() | ||
|
|
||
| # test mixed src |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split this to its own test
mne/viz/tests/test_3d.py
Outdated
| bem=sphere, | ||
| ) | ||
| # but you can ask for a specific brain surface, and | ||
| # no info is permitted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # no info is permitted | |
| # then omitting info is permitted |
| info, | ||
| trans=trans_fname, | ||
| subject="sample", | ||
| subjects_dir=subjects_dir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull out a shared_kwargs dict with these 4 params, to make clearer which params are different between each call to plot_alignment in this test
mne/viz/tests/test_3d.py
Outdated
|
|
||
|
|
||
| @testing.requires_testing_data | ||
| def test_plot_alignment_trans_erros(renderer, evoked): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def test_plot_alignment_trans_erros(renderer, evoked): | |
| def test_plot_alignment_trans_errors(renderer, evoked): |
mne/viz/tests/test_3d.py
Outdated
|
|
||
| @testing.requires_testing_data | ||
| def test_plot_alignment_info(renderer, evoked): | ||
| """Test basic alignment plotting with info, but no trans or other.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| """Test basic alignment plotting with info, but no trans or other.""" | |
| """Test plotting with info, but no trans, fwd, bem, or src.""" |
mne/viz/tests/test_3d.py
Outdated
| info, | ||
| trans, | ||
| subject="foo", | ||
| subjects_dir=subjects_dir, | ||
| surfaces=["brain"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see if you can dedup these param lists by pulling out a shared params dict and overriding / augmenting it as needed for each call.
|
@larsoner @drammock Hey Eric, something I noticed while doing this refactor is that our volumetric source space example in our testing data set does not have a value for the subject field, i.e. |
|
We could, but it's also pretty easy to add a instead, which might be preferable anyway since it avoids some boilerplate. There are some similar things here already Line 783 in 7ed5e27
|
What does this implement/fix?
This is a PR to simplify an overly long and complicated test in the testing suite for #D visualization. This refactor breaks testing of the function
mne.viz.plot_alignmentinto manageable chunks, separating tests of different parameters, and separating error checking from test plots (except where doing so would add extra up front instantiations).