-
Notifications
You must be signed in to change notification settings - Fork 23
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: Add notebook for MONAI with itk-elastix #176
ENH: Add notebook for MONAI with itk-elastix #176
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Looks good, one minor remark.
@HastingsGreer @aylward thoughts and suggestions appreciated |
Nicely done. Great, compact, and complete example. Serves as an example of how other methods can be used to create custom transforms. |
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.
LGTM!
For the record, the error at Notebook tests / Run notebooks on treebeard (pull_request) says:
|
@@ -0,0 +1,502 @@ | |||
{ |
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.
Line #14. from monai.apps import MedNISTDataset
Could a cell at the top please be added to install dependencies via pip? This improves reproduciblity and portability. We want the CI tests to pass.
Reply via ReviewNB
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.
Until we get to that point, we should install the dependencies here: https://github.com/InsightSoftwareConsortium/ITKElastix/blob/main/.binder/requirements.txt
Very nicely done! |
Also - these should probably be invertible transforms. You'll need to store the transform determined and used by elastix as a local var in the transform, and provide a method for applying its inverse. For info, see https://docs.monai.io/en/stable/_modules/monai/transforms/inverse.html |
f888411
to
2ca99c9
Compare
Thanks everyone for the suggestions! @thewtex, @N-Dekker I added pytorch and MONAI in.binder/requirements.txt. @dzenanz I agree that aesthetics play an important role. The interpolation was @aylward Great suggestion! However, currently we don't have yet a way to store elastix transforms in memory - we are working on it. I think that the other option would be to store this information in the |
@thewtex, it looks that it now fails because there is no |
@ntatsisk please add |
2ca99c9
to
39df7b4
Compare
Could it be InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#9? Maybe @tbirdso could comment. |
I am not certain whether the issues are related. I am investigating issues with Windows Python and Linux Python 3.11 builds in InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#9, but it looks like notebook checks in this PR are getting Linux Python 3.9 wheels which are not known to be affected:
It may be that |
Hi @ntatsisk , ITKElastix 0.14.4 has been released for compatibility with the ITK v5.3rc04.post4 release. Notebook checks are now passing again in the Would you please rebase these changes on |
39df7b4
to
29c1484
Compare
The notebooks seem to run now but this time it gives the above error. It happens when the notebook attempts to move the model to the GPU. Any ideas how to fix this? |
I think that GitHub actions runner machines don't have GPUs. The usual solution is to make code support CPUs, too. Usual way is described on SO. |
Hi @ntatsisk , yes, this is expected as Github CI runners do not have GPUs attached. There are two approaches we can take here:
EDIT: I like @dzenanz 's solution best -- if this is a notebook that could run on CPU then it should be amended to do so. |
Thank you for the suggestions. For some reason I believed that there was GPU support already here but clearly that is not the case. I think the decision if this and the future MONAI notebooks run on the CPU or the GPU boils down to what our goal is. Do we aim for complete real-world applications/problems with maybe >2D data or more minimal examples simply showcasing the possibilities with If we want to avoid GPU support in this repo, I can see also the solution of having two sets of notebooks: 1) simple ones that are tested (CPU) and 2) more elaborate ones that might use the GPU but are not tested. The latter could live also outside of this repo if it makes more sense. What are your thoughts? (.. cc @N-Dekker, @thewtex ) |
@ntatsisk thanks for working on this. My 2 cents on a path forward: Yes, we should have notebook that runs on CPU and can also utilize GPU. I think a good path forward is to support CPU for CI for the 2D. We can set up a GPU CI test in another PR. We can add the 3D notebook and just make a note that it should be used on a GPU system for a reasonable run time. |
With the help of Niels Dekker.
29c1484
to
71610a5
Compare
Great! I changed the notebook to use the GPU conditionally as @dzenanz pointed out. I also made the number of epochs dependent on whether the notebook runs on CPU or GPU. That way it gets tested in the CPU while the runtime is kept low. |
Hi @ntatsisk , is this ready to be merged? |
@tbirdso yes! :) |
Adds an example of how to combine itk-elastix as a pre-processing registration step for deep learning using MONAI.
Related to the discussion in #126.