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

[WIP] add t1 option in fslbetcrop module #62

Merged
merged 13 commits into from
Mar 1, 2024

Conversation

ThoumyreStanislas
Copy link
Contributor

add the option for a fsl bet / crop on a T1 images and create the test with Scilpy Fetcher

@AlexVCaron AlexVCaron mentioned this pull request Feb 22, 2024
@AlexVCaron AlexVCaron linked an issue Feb 22, 2024 that may be closed by this pull request
@AlexVCaron AlexVCaron added this to the nf-scil modules v1.0 milestone Feb 23, 2024
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.

This is on the right path, very good job 🚀 However, looking at the current tractoflow, there is a few more options to add as well for it to fit the use-case. We need to have a chat about it, because looking at the current structure, you'll end up adding imbricated if/else blocks, and I don't want that.

modules/nf-scil/betcrop/fslbetcrop/main.nf Outdated Show resolved Hide resolved
tests/modules/nf-scil/betcrop/fslbetcrop/nextflow.config 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.

Small things but good job! 🥇

modules/nf-scil/betcrop/fslbetcrop/main.nf Outdated Show resolved Hide resolved
modules/nf-scil/betcrop/fslbetcrop/meta.yml Outdated Show resolved Hide resolved
@AlexVCaron AlexVCaron removed a link to an issue Feb 27, 2024
@ThoumyreStanislas
Copy link
Contributor Author

@AlexVCaron for the moment the script works in this form but the nomenclature is not optimal. There are too many "image" files but I'm having trouble finding a good way to change that.

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.

Tiny changes, but then GTG ! I like this in this state, no need to complexify the module futher. We'll look back later if the naming need to change with the modality that gets betted.

tests/modules/nf-scil/betcrop/fslbetcrop/nextflow.config Outdated Show resolved Hide resolved
tests/modules/nf-scil/betcrop/fslbetcrop/nextflow.config 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.

Nice job! I noticed two things that I missed before, but otherwise, it is really nice! 💯

modules/nf-scil/betcrop/fslbetcrop/main.nf Show resolved Hide resolved
modules/nf-scil/betcrop/fslbetcrop/main.nf 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 🍻

@AlexVCaron AlexVCaron merged commit 887e6ee into scilus:main Mar 1, 2024
7 checks passed
@ThoumyreStanislas ThoumyreStanislas deleted the t1_betcrop branch June 13, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants