MRG, ENH: Abstracted volume registration again

[larsoner]

Another version of #9515, but with fewer tests modified. The relevant ones pass locally at least.

@alexrockhill feel free to test _USE_PREALIGN=False to see if it works better now!

[larsoner]

The key commit is 8794813

[alexrockhill]

So this issue is how you apply a reg_affine to an elec_image. I don't think you can while preserving the integer voxels values. So for the ieeg case, I think you have to use pre_align.

For the other case it would be nice to use the same code, but that seems like a large overhaul of tests and visualizations because they are not functionally equivalent.

[larsoner]

So this issue is how you apply a reg_affine to an elec_image. I don't think you can while preserving the integer voxels values

Sure you can, same way as you do for the SDR -- you use interpolation='nearest'

For the other case it would be nice to use the same code, but that seems like a large overhaul of tests and visualizations because they are not functionally equivalent.

This part I don't understand. What is the "other case" here, non-nearest (i.e., linear) case?

[alexrockhill]

You could move the coordinates to aligned space and make the elec_image that way so that it doesn't have to be registered but that throws a wrench in our API because you apply the reg_affine to the input image and then apply the SDR so that complicates things as well. The pre_align works great and I think is preferable API simplicity.

[larsoner]

Basically the commit uses resample(..., interpolation='nearest') but this is not exposed by Dipy so it just dups the code they use:

8794813#diff-780e284e0c926dd24e28047f79e10f0138e800e5a91a35b88c17689edd1909b8R1789

The morph tests suggest the prealign is slightly less accurate than the apply-then-SDR. Can you check to see if on this branch things look the same or better for your sEEG data?

[alexrockhill]

This part I don't understand. What is the "other case" here, non-nearest (i.e., linear) case?

The other case being the volumetric source space warp. That one can't just be switched to pre-align because it causes all the tests to fail.

[larsoner]

The pre_align works great and I think is preferable API simplicity.

I agree it's slightly simpler internally to use prealign, but users never see it. So if the prealign is less accurate (as the morph tests suggest) then I'd rather use our apply-then-SDR case...

[alexrockhill]

Basically the commit uses resample(..., interpolation='nearest') but this is not exposed by Dipy so it just dups the code they use:

8794813#diff-780e284e0c926dd24e28047f79e10f0138e800e5a91a35b88c17689edd1909b8R1789

The morph tests suggest the prealign is slightly less accurate than the apply-then-SDR. Can you check to see if on this branch things look the same or better for your sEEG data?

Ok, I misunderstood, I think this is great then! My bad just thinking within the Dipy API and not thinking to just apply interpolation='nearest'.

[larsoner]

Do you want to try porting over your reg-on-the-fly (and test that it still works!) and any other change(s) from #9515? If it all tests well at your end I'll push a commit to remove all prealign stuff, and port in the SourceMorph attribute updates from #9503 then we should be good to go hopefully!

I won't get to it until Tuesday, though, so if you want to take a shot at doing the above as well, feel free. Otherwise if you're more interested in moving on to the GUI stuff feel free to do that!

[alexrockhill]

Do you want to try porting over your reg-on-the-fly (and test that it still works!) and any other change(s) from #9515? If it all tests well at your end I'll push a commit to remove all prealign stuff, and port in the SourceMorph attribute updates from #9503 then we should be good to go hopefully!

I won't get to it until Tuesday, though, so if you want to take a shot at doing the above as well, feel free. Otherwise if you're more interested in moving on to the GUI stuff feel free to do that!

Sure, I can do that. I think for the reg-on-the-fly as you call it you have to change 4 tests but not too much (just loosen slightly). I'll take at Alex Gramfort's issue and then get to it.

[larsoner]

I think for the reg-on-the-fly as you call it you have to change 4 tests

I'm only talking about this tutorial change:

https://github.com/mne-tools/mne-python/pull/9515/files#diff-f802a9dfc7b418c33db03c47e5eb7353f3e124121ff07c4b867fa274d83d0d88R132-R134

No changes to internal code for "reg on the fly" (as opposed to hard-coded values)

[alexrockhill]

I think for the reg-on-the-fly as you call it you have to change 4 tests

I'm only talking about this tutorial change:

https://github.com/mne-tools/mne-python/pull/9515/files#diff-f802a9dfc7b418c33db03c47e5eb7353f3e124121ff07c4b867fa274d83d0d88R132-R134

No changes to internal code for "reg on the fly" (as opposed to hard-coded values)

Ah I thought you meant you didn't move the out_affine code back which fixes the MR-CT alignment. Ok checking now!

[alexrockhill]

Ok, the example runs at full resolution just fine and all the tests pass! And, thinking about it, it is consistent with how the affine registration is done (applying before each optimization). I'm happy, looks great to merge.

[larsoner]

We'll just have to see how long it takes to run on CircleCI. If it's many minutes, we might still want to hard-code the result (and just comment out the high-res / best version). On Tuesday I can add back the SourceMorph changes unless you beat me to it

[alexrockhill]

We'll just have to see how long it takes to run on CircleCI. If it's many minutes, we might still want to hard-code the result (and just comment out the high-res / best version). On Tuesday I can add back the SourceMorph changes unless you beat me to it

It was 2.5 minutes to run locally, that's okay right?

[larsoner]

It was 2.5 minutes to run locally, that's okay right?

That's probably too slow for CircleCI -- < 1 min is best for an example like this, and CircleCI is slower than most workstations so it's going to probably take 5+ minutes there if I had to guess (assuming you're not running on an underpowered laptop or anything)

[alexrockhill]

It was 2.5 minutes to run locally, that's okay right?

That's probably too slow for CircleCI -- < 1 min is best for an example like this, and CircleCI is slower than most workstations so it's going to probably take 5+ minutes there if I had to guess (assuming you're not running on an underpowered laptop or anything)

2:29 when run locally...

[larsoner]

This actually isn't as bad as I expected:

Total running time of the script: ( 2 minutes 10.853 seconds)

This might be a problem (<1.5 GB is better):

Estimated memory usage: 1778 MB

I'll push a commit to comment out the affine since it makes it much faster and memory efficient; locally I saw:

• 61.62 sec 1765.1 MB with zooms version / fec45f1
• 303.02 sec 3488.9 MB with zooms=None to get the affine
• 38.53 sec 1614.2 MB using the hard-coded affine
• 41.92 sec 830.6 MB using hard-coded affine plus np.asarray(img.dataobj) rather than img.get_fdata in several places (this allows smaller datatypes than np.float64 to be used in a few places)

@alexrockhill want to take one last look at the example and make sure everything is okay, then we merge? Here it is:

https://30428-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/clinical/10_ieeg_localize.html#sphx-glr-auto-tutorials-clinical-10-ieeg-localize-py

[alexrockhill]

I would use the zooms=dict(translation=5) version if you're going to comment it out. The hard-coded affine is in previous commits.

[alexrockhill]

Looks like there is issues with one too many "`" in the warning about zooms for the SDR

[larsoner]

I would use the zooms=dict(translation=5) version if you're going to comment it out. The hard-coded affine is in previous commits.

Okay, I thought at one point you said this was no longer necessary, but if it works at least as well why not! I'm planning to put in whatever affine I get from locally using this code, hopefully it matches well what was there before

[alexrockhill]

I would use the zooms=dict(translation=5) version if you're going to comment it out. The hard-coded affine is in previous commits.

Okay, I thought at one point you said this was no longer necessary, but if it works at least as well why not! I'm planning to put in whatever affine I get from locally using this code, hopefully it matches well what was there before

You can use zooms=None but that takes twice as long, the outputs are equivalent just trying to save you some time.

[larsoner]

Okay locally it works, I'll go ahead and merge to save the CIs some cycles. Thanks @alexrockhill !

[larsoner]

... I say that then make flake tells me there is a problem. I'll just let CIs finish.