Skip to content
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

Merged
merged 32 commits into from May 13, 2024

Conversation

karanphil
Copy link
Contributor

@karanphil karanphil commented Feb 12, 2024

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 the frf and fodf modules. Also, I realised that frf and dtimetrics did not use scil_dwi_extract_shell.py at the beginning to ensure only DTI shells are used, so I added it to both, just as in fodf for the ssst method. In summary:

  • Added a step to extract DTI shells in the reconst/dtimetrics module.
  • Added a step to extract DTI shells in the reconst/frf module, in the case of single-shell method. Also, added the option of multi-shell method.
  • Created a new reconst/shmetrics module to compute our typical metrics from whatever SH data, not only inside reconst/fodf.
  • Added the option of multi-shell method to reconst/fodf.

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

  • Create the tool:
    • Edit ./modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./modules/nf-scil/<category>/<tool>/meta.yml
  • Generate the tests:
    • Edit ./tests/modules/nf-scil/<category>/<tool>/main.nf
    • Edit ./tests/modules/nf-scil/<category>/<tool>/nextflow.config
    • Add test data locally for tests with the fork repository
    • Generate the test infrastructure and md5sum for all outputs
  • Ensure the syntax is correct :
    • Check indentation abides with the rest of the library (don't hesitate to correct others !)
    • Lint everything. Ensure your variables have good names.

@Manonedde Manonedde added the WIP label Feb 15, 2024
@Manonedde Manonedde linked an issue Feb 15, 2024 that may be closed by this pull request
@AlexVCaron AlexVCaron changed the title [WIP] Add msmt support to FODF and FRF (waiting for scilpy 2.0) [Improvement][WIP] Add msmt support to FODF and FRF (waiting for scilpy 2.0) Mar 8, 2024
@AlexVCaron AlexVCaron added this to the nf-scil v1.0 milestone May 6, 2024
@karanphil karanphil changed the title [Improvement][WIP] Add msmt support to FODF and FRF (waiting for scilpy 2.0) [Improvement] Add msmt support to FODF and FRF May 7, 2024
@karanphil
Copy link
Contributor Author

@AlexVCaron @arnaudbore This is ready for review!

Copy link
Contributor

@gagnonanthony gagnonanthony left a 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!

modules/nf-scil/reconst/fodf/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/reconst/fodf/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/reconst/frf/meta.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@gagnonanthony gagnonanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🍻

Copy link
Collaborator

@AlexVCaron AlexVCaron left a 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.

modules/nf-scil/reconst/dtimetrics/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/reconst/fodf/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/reconst/fodf/main.nf Show resolved Hide resolved
modules/nf-scil/reconst/fodf/meta.yml Outdated Show resolved Hide resolved
modules/nf-scil/reconst/frf/main.nf Show resolved Hide resolved
modules/nf-scil/reconst/frf/meta.yml Outdated Show resolved Hide resolved
modules/nf-scil/reconst/shmetrics/main.nf Show resolved Hide resolved
Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

Copy link
Contributor

@arnaudbore arnaudbore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG

@arnaudbore arnaudbore merged commit 5068159 into scilus:main May 13, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fodf metrics should be a module
5 participants