Pydeface module#373
Conversation
AlexVCaron
left a comment
AlexVCaron
left a comment
There was a problem hiding this comment.
Excellent work ! My comments and suggestions should get everything inline with our guidelines 🙂 Don't hesitate to get in touch here or via slack if you need my help !
| container { | ||
| workflow.containerEngine in ['singularity', 'apptainer'] && !(task.ext.singularity_pull_docker_container ?: false) | ||
| ? 'docker://poldracklab/pydeface:latest' | ||
| : 'docker.io/poldracklab/pydeface:latest' | ||
| } |
There was a problem hiding this comment.
Here, use the docker container definition directly. Support for apptainer and other environments is in the team's backlog and will be deployed over the whole library in the future.
| container { | |
| workflow.containerEngine in ['singularity', 'apptainer'] && !(task.ext.singularity_pull_docker_container ?: false) | |
| ? 'docker://poldracklab/pydeface:latest' | |
| : 'docker.io/poldracklab/pydeface:latest' | |
| } | |
| container "poldracklab/pydeface:latest" |
| cpus { (params.pydeface_cpus ?: 4) as Integer } | ||
| memory { params.pydeface_mem ?: '8 GB' } | ||
| time { params.pydeface_time ?: '2h' } |
There was a problem hiding this comment.
For resources, we use nf-core labels. They used to have a better definition of the kind of resources attached to each, we need to document that better on our side.
For this case I'd use the process_medium label:
| cpus { (params.pydeface_cpus ?: 4) as Integer } | |
| memory { params.pydeface_mem ?: '8 GB' } | |
| time { params.pydeface_time ?: '2h' } | |
| label "process_medium" |
| tuple val(meta), path(nifti) | ||
|
|
||
| output: | ||
| tuple val(meta), path("sub-${meta.subject}/ses-${meta.session}/anat/*_defaced.nii.gz"), emit: defaced |
There was a problem hiding this comment.
Is the bids-like output structure here needed for the PyDeface tool ? The group opted to keep the outputs of modules unstandardized, and manage the outputs conversion using publishDir at the pipeline level. You can see the docs for that standard here.
| sub="sub-${meta.subject}" | ||
| ses="ses-${meta.session}" |
There was a problem hiding this comment.
To be general, don't expect to have access to the subject or session here, but you'll always have access to the meta.id field. It should be used instead to name the output files. The conversion to BIDS will then be handled at file publishing by the pipeline, like stated above.
| if [[ "\$base" == *"_defaced" ]]; then | ||
| echo "[SKIP] Input already looks defaced: \$base" | tee -a "\$out_log" | ||
| cp "${nifti}" "\$out_file" | ||
| else |
There was a problem hiding this comment.
This main condition should not be required. The including subworkflow or pipeline will have the job of filtering out those data points
| Groovy Map containing sample information. | ||
| Expected keys for this module currently include `subject` and `session`, | ||
| for example `[ subject: '01', session: '001' ]`. | ||
| - sub-${meta.subject}/ses-${meta.session}/anat/*_defaced.nii.gz: {} |
There was a problem hiding this comment.
This block should contain a short description of the image content and descriptors. Here's a (recent module](
) that contains everything formatted as we intend| versions_pydeface: | ||
| - - "${task.process}": | ||
| type: string | ||
| description: The name of the process. | ||
| - "pydeface": | ||
| type: string | ||
| description: The name of the tool. | ||
| - "pydeface_version": | ||
| type: string | ||
| description: The version of PyDeface used by the process. |
There was a problem hiding this comment.
Refer to the module linked above to replace the versions block. You can use the exact same one, copy-pasted !
| topics: | ||
| versions: | ||
| - - "${task.process}": | ||
| type: string | ||
| description: The name of the process. | ||
| - "pydeface": | ||
| type: string | ||
| description: The name of the tool. | ||
| '"versions"': | ||
| - versions.yml: | ||
| type: string | ||
| description: The name of the process. |
There was a problem hiding this comment.
Is this default ? I have to get a look at newest updates to the nf-core guidelines, we should update documentation on our end to align with the latest additions 😅
There was a problem hiding this comment.
You can remove most of the content of this file, defaults like trace and timeline are already set by overhead test configurations. Refer here for the content that should appear (if you need some, maybe you could also just remove the file).
2 participants
Describe your changes
This PR adds a new
PYDEFACEmodule undermodules/nf-neuro/anonymization/pydeface.The module runs PyDeface on anatomical MRI NIfTI files to remove facial structures for anonymization.
It takes a tuple containing sample metadata and an input NIfTI image, creates a BIDS-like output path using subject and session metadata, and emits the defaced NIfTI image, log files, and software version information.
List test packages used by your module
The module test uses the nf-neuro
LOAD_TEST_DATAsubworkflow to provide a test T1w NIfTI image.Test data/package used:
T1w.zipThe test checks that:
PYDEFACEprocess completes successfully*_defaced.nii.gzpatternChecklist before requesting a review
./modules/nf-neuro/<category>/<tool>/main.nf./modules/nf-neuro/<category>/<tool>/meta.yml./modules/nf-neuro/<category>/<tool>/environment.yml--> we use the PyDeface container directly./modules/nf-neuro/<category>/<tool>/tests/main.nf.testmain.nf.test.snapsnapshotsprettierandeditorconfig-checkerto fix common syntax issuesnf-core modules lintand fix all errors --> there are still a few errors and warnings