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

[ENH, MRG] Add coregistration video and execute GUI in tutorial to show window & add show and block to GUIs #10802

Merged
merged 18 commits into from
Jun 24, 2022

Conversation

alexrockhill
Copy link
Contributor

@alexrockhill alexrockhill commented Jun 22, 2022

Fixes #9451, fixes #10801.

@alexrockhill alexrockhill changed the title [ENH, MRG] Add coregistration video and execute GUI in tutorial to show form [ENH, MRG] Add coregistration video and execute GUI in tutorial to show window & add show and block to GUIs Jun 22, 2022
@alexrockhill
Copy link
Contributor Author

Alright! A bit of the code is a tiny bit inelegant but that will be fixed with future PRs that slightly refactor the window classes so that they don't depend on the 3D renderer, harmonizing between the coreg & brain GUIs and the ieeg_locate GUI. For now, I think this is great, the video looks good and the GUI is scraped and appears in the docs, real progress I think https://output.circle-artifacts.com/output/job/754bf05f-35bd-4deb-8050-67f788191330/artifacts/0/dev/auto_tutorials/forward/20_source_alignment.html.

@agramfort
Copy link
Member

@alexrockhill in the video you pick the fiducials too high. In this subject the LPA and RPA should be just just below the targus

@alexrockhill
Copy link
Contributor Author

@alexrockhill in the video you pick the fiducials too high. In this subject the LPA and RPA should be just just below the targus

Ok, I will reshoot picking as in here https://www.fieldtriptoolbox.org/faq/how_are_the_lpa_and_rpa_points_defined/

@alexrockhill
Copy link
Contributor Author

Ok new video is up, looks good https://output.circle-artifacts.com/output/job/a249d7a8-3e0b-4508-9864-8bca3bc5fa25/artifacts/0/dev/auto_tutorials/forward/20_source_alignment.html#defining-the-headmri-trans-using-the-gui. I did notice that there was a reference to toggling head points that is defunct so I'll fix that.

@alexrockhill
Copy link
Contributor Author

I noticed that the text instructions were out-of-date so this fixes those as well.

@agramfort
Copy link
Member

agramfort commented Jun 23, 2022 via email

@alexrockhill
Copy link
Contributor Author

Ok, I'll give the video one more shot

@alexrockhill
Copy link
Contributor Author

@agramfort, if you have time for one more look, sorry for the bad takes, you can playback at double speed and it's only two minutes 😄 https://youtu.be/ALV5qqMHLlQ

@cbrnr
Copy link
Contributor

cbrnr commented Jun 23, 2022

Maybe something for the text (I don't want you to redo the video again), but I have been wondering where the exact location of a fiducial marker actually is. Is it at some vertex? Or the center of mass inside the shape? Probably worth mentioning because that shape has a non-negligible shape.

@alexrockhill
Copy link
Contributor Author

I was going off https://www.fieldtriptoolbox.org/faq/how_are_the_lpa_and_rpa_points_defined/ @cbrnr, we could link that in the tutorial

@cbrnr
Copy link
Contributor

cbrnr commented Jun 23, 2022

No, I meant which point in that fiducial marker (two stacked pyramids) is the origin? Should I use the top vertex or the bottom one to set the fiducial location?

@alexrockhill
Copy link
Contributor Author

I think it's the center of mass of the shape... does that help?

@cbrnr
Copy link
Contributor

cbrnr commented Jun 23, 2022

Sure! We should mention this.

@alexrockhill
Copy link
Contributor Author

@cbrnr, I added that info in the text

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.

video is great !

are all the changes in this PR related? I see you touch stuff in GUI code and I want to know how deeply I need to review this

@alexrockhill
Copy link
Contributor Author

alexrockhill commented Jun 23, 2022

If you could give it 5 minutes that would be really helpful but I'm not too worried about things breaking since I'm mostly just using things that have worked in the other GUI for the coreg and ieeg_locate GUIs to harmonize. I abstracted the scraper to work for the coreg GUI as well so that it could be displayed inline and then I added show and block so that it wouldn't block the execution in the tutorial. Those are so that the code actually executes in the tutorial which I think is really important for maintenance. I added show and block to the ieeg_locate GUI while I was at it for symmetry. All of those are pretty minor changes, I'm not too worried about them breaking because they worked on the other GUI and I'm just harmonizing. Lastly, I fixed the wording suggesting that you could only launch coreg from the CLI which I think is a bit out-of-date per @hoechenberger 's issue.

@larsoner
Copy link
Member

@agramfort is happy with the video, I'm happy with the code, and I don't see any unresolved comments so in it goes, thanks @alexrockhill !

@larsoner larsoner merged commit db6c822 into mne-tools:main Jun 24, 2022
@alexrockhill alexrockhill deleted the vid3 branch June 24, 2022 14:11
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.

[ENH] Add video tutorial for coregistration Recommended way to launch the coregistration GUI
4 participants