Skip to content

[Module] Freesurfer synthregistration#54

Open
anroy1 wants to merge 34 commits intoscilus:mainfrom
anroy1:registration
Open

[Module] Freesurfer synthregistration#54
anroy1 wants to merge 34 commits intoscilus:mainfrom
anroy1:registration

Conversation

@anroy1
Copy link
Contributor

@anroy1 anroy1 commented Jan 19, 2024 •
edited
Loading

Describe your changes

Initial module PR for registration tool using Freesurfer synthmorph

Say which test data are used by your module

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.

@AlexVCaron
Copy link
Collaborator

AlexVCaron commented Jan 24, 2024

@nf-scil-bot fix linting

1 similar comment
@AlexVCaron
Copy link
Collaborator

AlexVCaron commented Feb 7, 2024

@nf-scil-bot fix linting

@AlexVCaron AlexVCaron changed the title Registration Freesurfer registration Mar 7, 2024
@AlexVCaron AlexVCaron changed the title Freesurfer registration Add Freesurfer registration module Mar 7, 2024
@AlexVCaron AlexVCaron changed the title Add Freesurfer registration module [Module] Freesurfer registration Mar 7, 2024
@anroy1 anroy1 force-pushed the registration branch 2 times, most recently from 8148b67 to 00fc10f Compare April 9, 2024 19:22
@anroy1 anroy1 force-pushed the registration branch 2 times, most recently from 6c16d91 to ec7d848 Compare April 26, 2024 01:33
AlexVCaron
AlexVCaron requested changes Jun 13, 2024
View reviewed changes
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.

Great job, almost there ! 💪🏻 To be consistent with the remainder of the library, I would rather this module outputs standard file formats. You're there with Nifti, but lta should be converted to mat/txt, the easiest (mat prefered since binary).

@anroy1
Copy link
Contributor Author

anroy1 commented Jun 13, 2024

Regarding your comment on .lta format, it would be possible by adding the lta_convert function, as synthmorph affine output only .lta in RAS orientation. Making this module able to combine would require then to change the docker container: freesurfer/synthmorph --> freesurfer/freesurfer, which then require the licence management. This is mainly why I decided to split to create a separate module registration/convert. This is something we can discuss and think through.

@AlexVCaron
Copy link
Collaborator

AlexVCaron commented Jun 14, 2024

Maybe one day we'll be able to fuse the containers, but for now, let's keep it like that. Add a short paragraph to the description in the meta referring to the conversion module.

@anroy1 anroy1 force-pushed the registration branch 2 times, most recently from bee294e to c1e190f Compare August 8, 2024 20:10
@anroy1 anroy1 force-pushed the registration branch 2 times, most recently from 8d014ce to 7e2f8d1 Compare September 2, 2024 16:43
@anroy1 anroy1 force-pushed the registration branch 2 times, most recently from 62b9daf to b188fbc Compare September 7, 2024 18:59
@anroy1 anroy1 changed the title [Module] Freesurfer registration [Module] Freesurfer synthregistration Sep 7, 2024
anroy1 added 28 commits September 16, 2024 21:09
@anroy1
2nd fix for tests
d2d0e4a
@anroy1
quick fix
41131a8
@anroy1
test naming fix
d82ae82
@anroy1
fix scilpy fetcher path
7c274d0
@anroy1
fix tests parameters
1f34c3a
@anroy1
[WIP] modify module and test to manage freesurfer license
41f8859
@anroy1
fix test data, freesurfer license
b51f537
@anroy1
[WIP] improve test
bacd6cb
@anroy1
Adjust nextflow config and output format
496ac48
@anroy1
Swith transform format to freesurfer standard
b632962
@anroy1
run prettier
077f420
@anroy1
Remove license as module do not require it and update parameters with…
98209d4 
… newer version
@anroy1
Switch transform output to fs .nii.gz format
d47cbed
@anroy1
Migrate tests to nf-tests
ce493f8
@anroy1
fix nf-tests
1cfe90d
@anroy1
Remove pytests
2aff22f
@anroy1
Run prettier
6df7ba4
@anroy1
Modify tests config for remote tests
2967f20
@anroy1
Fix test assertion for name only
d094e65
@anroy1
Rerun tests and fix linting
b95c032
@anroy1
adjust process parameters for tests locally
3299ec0
@anroy1
[wip] fix alex comments 1/2
afd5884
@anroy1
Add precisions regarding convertion on meta.yml
6c8468b
@anroy1
Handling threads in script
b46749c
@anroy1
change container version
48e5584
@anroy1
Use nifti md5sum assertion
614dd42
@anroy1
test with niftiMD5SUM
a1f8f2d
@anroy1
Revert "Change org_path"
6984f7a 
This reverts commit b188fbc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexVCaron AlexVCaron AlexVCaron requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anroy1 @AlexVCaron