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

BUG: Fix bug with plot_alignment checks #10309

Merged
merged 4 commits into from Feb 8, 2022

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Feb 8, 2022

Closes #10308

@dasdiptyajit feel free to see if this fixes your problem

Our input validation for trans was deficient, this fixes it. cc @alexrockhill since I think this was your code at some point.

It also touches all tutorials that use plot_alignment to see if it breaks anything...

@alexrockhill
Copy link
Contributor

Ah, I'm very sorry if I broke the forward solution plotting in my refactoring, it wasn't what I was focused on so I didn't check it as thoroughly. I'll review as soon as I can today.

mne/viz/_3d.py Outdated Show resolved Hide resolved
Copy link
Member

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

LGTM

merge if happy @larsoner

@@ -8,7 +8,8 @@
Here we compute the resting state from raw for data recorded using
a Neuromag VectorView system and a custom OPM system.
The pipeline is meant to mostly follow the Brainstorm :footcite:`TadelEtAl2011`
`OMEGA resting tutorial pipeline <bst_omega_>`_.
`OMEGA resting tutorial pipeline
<https://neuroimage.usc.edu/brainstorm/Tutorials/RestingOmega>`__.
Copy link
Contributor

Choose a reason for hiding this comment

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

Too many underscores?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was intentional, it's an anonymous hyperlink in RST (which IIUC is for when you don't plan to reuse the same link name later)

@alexrockhill
Copy link
Contributor

Looks good, let me check all the plot alignment tutorial plots vs 0.24 though, one minute

@larsoner
Copy link
Member Author

larsoner commented Feb 8, 2022

Do not compare to 0.24 but to main

https://mne.tools/dev/auto_examples/preprocessing/movement_detection.html?highlight=plot_alignment

Bug exists there, too, so not due to this PR

@larsoner
Copy link
Member Author

larsoner commented Feb 8, 2022

Let's fix those separately (#10312) and just fix things here that this PR breaks, if any

@alexrockhill
Copy link
Contributor

Ok, good to merge by me, separate PRs sound good

@larsoner larsoner merged commit 6107c3a into mne-tools:main Feb 8, 2022
@larsoner larsoner deleted the plot_alignment branch February 8, 2022 21:15
@larsoner
Copy link
Member Author

larsoner commented Feb 8, 2022

Okay, I'll look into #10312 tomorrow hopefully, thanks for the quick look @alexrockhill !

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.

misalignment of source space in forward sol
4 participants