-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Affine registration #7421
base: main
Are you sure you want to change the base?
Affine registration #7421
Conversation
added exp input as measure of thoroughness got rid of off by one errors added previous closed commit
|
@jboulanger This is a huge amount of work; thank you very much! @jni (who guided #3544) and I have been discussing this feature ever since writing Elegant SciPy, and this puts us significantly closer to that goal. I would like for @jni to review, and @JDWarner has kindly offer to as well. This change is significant enough that we may want to dedicate the next developer call to it, and perhaps you want to join to discuss. |
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.
Just took a quick, superficial look at this huge PR!
invariant entropy measure of 3D medical image alignment. | ||
Pattern Recognition 32(1):71-86 | ||
:DOI:`10.1016/S0031-3203(98)00091-0` | ||
invariant entropy measure of 3D medical image alignment. |
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.
When did the syntax/convention change? Just curious. 🙏
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.
Hello, Thank for the modifications. I am not totally sure which syntax/convention you are referring to. Is it in the docstring? I propose a change in simple_metrics which is a bit a side effect of the main proposal. This is to have a uniform support of weights in the registration code across the two implemented methods. I hope this is backward compatible and doesn't break anything.
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.
I'm referring to the extra indent, which I also noticed lines 18-19 of doc/examples/registration/plot_register_affine.py
. Perhaps the formatting got fixed/taken care of by the pre-commit hook, in which case you didn't actually decide on it. I was just curious about the change because, up until now(?), we would write:
.. [1] Juan Nunez-Iglesias, Stefan van der Walt, and Harriet Dashnow. Elegant
SciPy: The Art of Scientific Python. 1st. O'Reilly Media, Inc.,
2017. isbn: 1491922877, 9781491922873.
and not
.. [1] Juan Nunez-Iglesias, Stefan van der Walt, and Harriet Dashnow. Elegant
SciPy: The Art of Scientific Python. 1st. O'Reilly Media, Inc.,
2017. isbn: 1491922877, 9781491922873.
Co-authored-by: Marianne Corvellec <[email protected]>
Co-authored-by: Marianne Corvellec <[email protected]>
Co-authored-by: Marianne Corvellec <[email protected]>
I have been exchanging a few messages with @jni on zulip before putting this together. However I am aware that the proposition diverged significantly from the original discussion that happened during #3544. I tried to balance usability and flexibility and also wanted to keep the multi-resolution factorized across the two methods. |
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.
Thank you for the contribution @jboulanger! What an awesome first PR!
This feature is one I have long anticipated and am excited to see in scikit-image
.
My review will be in 2 parts; this first set of comments is mostly stylistic from a detailed read-through to get the ball rolling. In the near future, I will follow up after digging in further with testing on real images in both 2 and 3 dimensions, as well as some performance profiling.
Thanks again!
from ._affine import ( | ||
affine, | ||
studholme_affine_solver, | ||
lucas_kanade_affine_solver, |
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.
For both solver functions, from a forward-looking naming convention standpoint, we likely want to tweak the names. To optimize the discoverability and intuitiveness of the namespace, perhaps we should consider function names beginning with either their linked registration type or purpose.
So for 'affine'
as here, we could go with either:
affine_solver_studholme
(linking the function to affine), or
solver_affine_studholme
(making it clear this function is a solver, then that it's linked to affine)
Either would be reasonable at the moment and I'd welcome input from the team as to their preference. I have a small preference to starting with solver_
so it's clear the group of functions is specific to that purpose.
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.
I'll wait a bit for more feedback on this. I have no big preference.
skimage/registration/_affine.py
Outdated
matrix : ndarray | None | ||
Intial guess for the homogeneous transformation matrix | ||
model: str | ||
Motion model |
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.
The docstrings for each solver specify that we can use the model options "affine", "translation" or "euclidean", but most users would interact with this option at the level of this function so we should make that especially clear in this docstring.
As a lower bound, give them the options: Motion model: "affine", "translation" or "euclidean".
In addition, it might be wise to spend a couple sentences expanding on the difference/constraints of a model limited to translation or euclidean registration alone versus affine registration so a user with less experience could choose without searching elsewhere.
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.
I added the following to the affine documentation:
model: str
Motion model: "translation", "euclidean" or "affine" where euclidean motion
corresponds to rigid motion (rotation and translation) and affine motion can
represent change in scale, shear, rotation and translations.
Let me know if this is the right place and if this is better documentation in general.
Thank you for the very informative feedback! |
Description
Intensity based affine registration of image pairs.
The API tries to take inspiration from the PR3455, however I found it not very user friendly to pass callback for handling euclidean or translation motion model. Moreover the logic is different between the two solvers and could not find a way to conciliate the previously discussed API with the added solver. PR3455 was relying on transforms in the example to handle restricted model (euclidean) which is not compatible with ndi.affine_transform, here we stick to ndi.affine_transform to ba able to handle 3D images.
Comparison bewteen the solvers and cv2.findTransformECC. The 2nd row display the registration error map.
![image](https://private-user-images.githubusercontent.com/3415561/330491013-a99c3bfa-6be5-4c36-93c9-3e4f96ac900b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkzODkyNTQsIm5iZiI6MTcxOTM4ODk1NCwicGF0aCI6Ii8zNDE1NTYxLzMzMDQ5MTAxMy1hOTljM2JmYS02YmU1LTRjMzYtOTNjOS0zZTRmOTZhYzkwMGIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDYyNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA2MjZUMDgwMjM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MjFkYjhkNDg2OWQ1NGNmMDkxYTIxNjA5ZjhjN2NlNTdhNDBiNTdjZGQwZjI3MmY3MzEwYzM3NjEyYTRlNGJmYyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.WQUHeDgkyYi1jUOeSvEZ2A3MFdA6rj7PqLzAWPEwjWw)
Registration of a brain MRI volume
![image](https://private-user-images.githubusercontent.com/3415561/330493289-e127c85f-8432-4c7e-97a5-2ac4867117fb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkzODkyNTQsIm5iZiI6MTcxOTM4ODk1NCwicGF0aCI6Ii8zNDE1NTYxLzMzMDQ5MzI4OS1lMTI3Yzg1Zi04NDMyLTRjN2UtOTdhNS0yYWM0ODY3MTE3ZmIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDYyNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA2MjZUMDgwMjM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9Y2MyMzYwZWZmNGQ3ZWIwZTRlOTQ3N2RiZTlhOTYyMGVmOTExNTRmZDc1YmMyODFiM2FiNGY4YmMzZjUxZTBmYiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.sl4yaIrMR8SmjtKW93gVX__FydutEAIK7C5_rp5C6t8)
TODO
References
Checklist
./doc/examples
for new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.