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
[Improvement] Add msmt support to FODF and FRF #72
Conversation
@AlexVCaron @arnaudbore This is ready for review! |
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.
Amazing job! 💯 I wrote more questions/comments than changes, you did a great job!
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! 🍻
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.
It's going along well 🚢 Great job @karanphil, I'm amazed how quick you got on board ! I have a few comments that I would like fixed before we integrate though.
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 🚢
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.
GTG
Describe your changes
What was supposed to be a quick change ended up being big. I created a module
shmetrics
, to give the possibility to compute metrics from SH files coming from anywhere. I added the option to used the msmt-fODF script in thefrf
andfodf
modules. Also, I realised thatfrf
anddtimetrics
did not usescil_dwi_extract_shell.py
at the beginning to ensure only DTI shells are used, so I added it to both, just as infodf
for the ssst method. In summary:For these four modules, all the tests were updated to the new way. Scilpy 2.0 is required so I also changed the container to scilus:2.0.0.
Say which test data are used by your module
This is complicated right now...
Checklist before requesting a review
./modules/nf-scil/<category>/<tool>/main.nf
./modules/nf-scil/<category>/<tool>/meta.yml
./tests/modules/nf-scil/<category>/<tool>/main.nf
./tests/modules/nf-scil/<category>/<tool>/nextflow.config