-
Notifications
You must be signed in to change notification settings - Fork 231
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
Add GiftiLoader extension ➡️ module GiftiLoader added to SlicerNeuro #1952
base: main
Are you sure you want to change the base?
Conversation
Looks handy - just a few comments: Here you should use Also these shouldn't be installed during module discovery or they will be run at application startup and will slow things down. Instead they should only be installed when needed (i.e. when the user invokes the functionality). Your tests look helpful, but there are hardcoded paths. It would be good if you could register some SampleData that can be downloaded for the tests both for regression testing and so people know what you are able to support. https://github.com/mcespedes99/SlicerGiftiLoader/blob/main/GiftiLoader/GiftiLoader.py#L791 |
Is there a specific reason for not using the gifti reader in ITK? We would not need to install extra packages and the implementation would be probably a bit simpler. If there is some limitation of the ITK implementation that disqualifies it then we should let ITK developers know about it. |
Is it only (or mainly) used with Freesurfer output? If yes then it could make sense to add it to the Freesurfer extension rather than creating a new extension for it. |
In regards to the first comment, is there any specific place I should put the installation of the modules then? @pieper maybe inside the init function of the Widget class? @lassoan, in regards to the ITK implementation, I'm not so familiar with the ITK implementation but I'll try to take a look and let you know. If you could point me to something in particular, I'd appreciate it. The module itself is able to load any gifti files from a BIDS directory, it's not specifically related to freesurfer. For example, it also works with fmriprep or hippunfold results! |
I think a method like Like what's done here: There's some documentation in development here, maybe we can flesh it out to describe best practices. |
https://simpleitk.readthedocs.io/en/v1.2.4/Documentation/docs/source/IO.html |
@mcespedes99 Maybe the GIFTI reader is only exposed in ITK-Python and not in SimpleITK. Would you mind posting a question about this to the ITK forum (and post the link here for reference)? |
Thanks for the post. I've added some comment there. In the meantime, I had a look at GIFTI format and it seems that it is strictly neuroimaging format, so it would make sense to not start a new extension for it but add it to https://github.com/Slicer/SlicerNeuro extension to make it easier to find for users and to reduce maintenance and support overhead. If you want you can get direct write access to the repository so that you can make changes without going through review. |
@lassoan sounds good!! I'll make a fork and a PR there |
Just opened a PR in the SlicerNeuro repo (Slicer/SlicerNeuro#1). I updated the installation as suggested as well as an update of the tests to register my images to the MRHead sample data. The installation of the packages only takes a little moment for me but I still added a progress bar that tells the user what is going on. Still no updates on the ITK questions I think. Please let me know if you have any further suggestions. I haven't compile the SlicerNeuro updates, so I would wait until this is tested to merge my changes. Probably will do it tomorrow during the morning. Thanks for all the help! |
Squash later
Following the suggestion from @lassoan, now I'm requesting to push my changes to SlicerNeuro. Then I just deleted the GiftiSlicer.s4ext and edited the SlicerNeuro one. I'm waiting for the PR to SlicerNeuro to be approved. I guess that as soon as that happens, this should be good to go. |
For reference, here is the pull request: |
@mcespedes99 Additional comment were provided at Slicer/SlicerNeuro#1 (review), when you have a chance could you address those ? |
Hi! Sorry, I've been busy with other things but it should be soon! Sorry for the delay |
Thanks for the follow up 🙏 In the meantime, let us know if you have any questions. |
@lassoan It turns this pull request will not be integrated. Instead, we need to finalize the following: |
New extension
3d-slicer-extension
GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter3d-slicer-extension
in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topicsSettings
and in repository settings uncheckWiki
,Projects
, andDiscussions
(if they are currently not used)About
in the top-right corner of the repository main page and uncheckReleases
andPackages
(if they are currently not used)This extension only contains 1 module, so I don't have an explicit description of each module but I do mention (on the gitub repo page) that it only has 1 module.