-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[WIP, ENH] Abstracted volume registration #9515
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
Conversation
|
After some testing I added which should give the equivalent outcome to but in fact doesn't, here is the stack trace from the tests I wrote the matching code by taking examples and matching the output of the shapes and affines to attributes of the This suggests that the code that uses I think the accuracy of the affine registration and SDR pipeline in this PR is superior to |
This makes sense, and I agree this is strong evidence that it works!
Here I think you sell the current code a bit short. That code has been around for a long time, and had probably half a dozen bugs fixed. This has led some some cruft, as you notice, but it also has led to new unit tests each time to check to make sure things are working properly. Hence the failing tests -- especially the round-trip distance one -- suggest that something isn't as good in the current implementation. To me there are two issues / potential differences between this (and the custom code in From what you're saying, it sounds to me like you have two objections to #9503:
Is that right? I agree we should improve the rigid/affine alignment (1), and if this PR does that then it seems to be on the right track. But I'm not sure whether or not My suggestion to get to the bottom of it is to look at why the tests fail, and see if they're fixed by not using apply-affine-then-compute-SDR rather than prealign-via-compute -- and then decide whether or not they actually show something is working functionally better or not (they really should since they were introduced to fix bugs, but who knows!). |
* upstream/main: MAINT: Speed up CIs (mne-tools#9518) Update tools (mne-tools#9517) FIX: Fix simulate_evoked and apply_forward (mne-tools#9513) FIX: Mayavi (mne-tools#9512)
|
Sounds good, thanks for the feedback, I'll take a look into the failing tests. |
|
Actually I think your snippet above just needs a small tweak, I think I almost have it working. Give me 10 minutes... |
Hey yay teamwork, take your time! |
|
Whenever we're settled here, I want to port the remaining |
So I think it's clear that although the prealign has a bit weirder behavior, it is the only practical option for warping an indexed that I'm not super inclined to look into fixing, but since the differences are minor maybe you could do that last fix @larsoner. Alternately, I'm about to push a version that splits the SDR morph in |
|
Just running the tutorial on the new new PR but looks like it works |
This splits up #9503 to just refactor the volume registration for CT/MR without combining it with the existing SDR morph for volume source estimates.
This won't work until mne-tools/mne-misc-data#7 merges.
Needs
_USE_PREALIGNand equiv in morph.py : one should really work for both (differences are minor)