Chore/niagara docs #18
Conversation
WalkthroughThis update enables Nextflow DSL 2 across all workflow scripts, standardizes resource specifications in the Niagara profile, and introduces a new workflow for collecting and combining volumes tailored for Niagara. The README is expanded with detailed Niagara-specific setup and execution instructions, including handling known issues. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nextflow (Niagara)
participant FileSystem
participant collectVolumes
participant combineVolumes
User->>Nextflow (Niagara): Start workflow
Nextflow (Niagara)->>FileSystem: List NIfTI files in output/labels/majorityvote
loop For each NIfTI file
Nextflow (Niagara)->>FileSystem: Extract label from filename
Nextflow (Niagara)->>FileSystem: Check for corresponding CSV in input/atlases
alt CSV exists
Nextflow (Niagara)->>collectVolumes: Pair NIfTI + CSV
else CSV missing
Nextflow (Niagara)->>collectVolumes: Pair NIfTI + placeholder
end
end
collectVolumes-->>Nextflow (Niagara): Collected volumes
Nextflow (Niagara)->>combineVolumes: Combine collected volumes
combineVolumes-->>Nextflow (Niagara): Final combined output
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
validateInputDirectoryStructure.nf (1)
1-1: Fix formatting: Remove leading space.The DSL2 directive should not have a leading space for consistency with other files.
- nextflow.enable.dsl=2 +nextflow.enable.dsl=2README.md (5)
30-30: Fix markdown link fragment case sensitivity.The link fragment should match the actual header case for proper navigation.
-These are not requirements if running on Niagara [see below](#Running-on-Niagara) +These are not requirements if running on Niagara [see below](#running-on-niagara)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Link fragments should be valid
Expected: #running-on-niagara; Actual: #Running-on-Niagara(MD051, link-fragments)
196-196: Add missing comma for clarity.-> As of writing this openjdk/17.0.9 was the latest version on Niagara. +> As of writing this, openjdk/17.0.9 was the latest version on Niagara.🧰 Tools
🪛 LanguageTool
[uncategorized] ~196-~196: Possible missing comma found.
Context: ...17.0.9 ``` > [!NOTE] > As of writing this openjdk/17.0.9 was the latest version o...(AI_HYDRA_LEO_MISSING_COMMA)
201-201: Fix duplicate word and add missing comma.-To ensure submission to SLURM the the Niagara profile must be used. +To ensure submission to SLURM, the Niagara profile must be used.🧰 Tools
🪛 LanguageTool
[uncategorized] ~201-~201: Possible missing comma found.
Context: ...and on Niagara To ensure submission to SLURM the the Niagara profile must be used. T...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~201-~201: Possible typo: you repeated a word.
Context: ... Niagara To ensure submission to SLURM the the Niagara profile must be used. This is p...(ENGLISH_WORD_REPEAT_RULE)
203-203: Remove vim command artifact.The
:wappears to be an accidental vim command insertion.-Other useful flags to pass are `-bg` to run in background and `-resume` to resume processing if there was an interuption. :w +Other useful flags to pass are `-bg` to run in background and `-resume` to resume processing if there was an interruption.🧰 Tools
🪛 LanguageTool
[uncategorized] ~203-~203: Loose punctuation mark.
Context: ...processing if there was an interuption. :w ```bash nextflow run -bg magetbrain.n...(UNLIKELY_OPENING_PUNCTUATION)
210-211: Fix grammar issues in bug description.-> A bug when running on Niagara requires and additional script to be run to collect volumes -> This can be done on the login node without submitting job to SLURM +> A bug when running on Niagara requires an additional script to be run to collect volumes +> This can be done on the login node without submitting a job to SLURM🧰 Tools
🪛 LanguageTool
[uncategorized] ~210-~210: “and” (in addition to, following this) seems less likely than “an” (indefinite article before a vowel sound).
Context: ... A bug when running on Niagara requires and additional script to be run to collect ...(AI_HYDRA_LEO_CP_AND_AN)
[uncategorized] ~211-~211: Possible missing article found.
Context: ...ne on the login node without submitting job to SLURM ```bash nextflow run collect_...(AI_HYDRA_LEO_MISSING_A)
magetbrain.nf (1)
254-256: Consider adding error handling for regex matching edge cases.The regex pattern
/_label_([\w]+)\.nii.gz/assumes a specific filename format. While the code handles non-matches by returningnull, consider adding validation to ensure the extracted label is valid.Consider adding validation for the extracted label:
.map { a_file -> def matcher = a_file.name =~ /_label_([\w]+)\.nii.gz/ if (matcher.find()) { def label = matcher.group(1) + if (!label || label.trim().isEmpty()) { + log.warn "Empty label extracted from file: ${a_file.name}" + return null + } def csvFile = file("${params.inputDir}/atlases/volume_labels_${label}.csv")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(2 hunks)collect_and_combine_volumes.nf(1 hunks)collect_and_combine_volumes_niagara.nf(1 hunks)magetbrain.nf(3 hunks)nextflow.config(2 hunks)validateInputDirectoryStructure.nf(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
30-30: Link fragments should be valid
Expected: #running-on-niagara; Actual: #Running-on-Niagara
(MD051, link-fragments)
🪛 LanguageTool
README.md
[style] ~185-~185: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...correct modules need to be loaded. This needs to be done every time. > [!IMPORTANT] > D...
(REP_NEED_TO_VB)
[uncategorized] ~196-~196: Possible missing comma found.
Context: ...17.0.9 ``` > [!NOTE] > As of writing this openjdk/17.0.9 was the latest version o...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~201-~201: Possible missing comma found.
Context: ...and on Niagara To ensure submission to SLURM the the Niagara profile must be used. T...
(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~201-~201: Possible typo: you repeated a word.
Context: ... Niagara To ensure submission to SLURM the the Niagara profile must be used. This is p...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~203-~203: Loose punctuation mark.
Context: ...processing if there was an interuption. :w ```bash nextflow run -bg magetbrain.n...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~210-~210: “and” (in addition to, following this) seems less likely than “an” (indefinite article before a vowel sound).
Context: ... A bug when running on Niagara requires and additional script to be run to collect ...
(AI_HYDRA_LEO_CP_AND_AN)
[uncategorized] ~211-~211: Possible missing article found.
Context: ...ne on the login node without submitting job to SLURM ```bash nextflow run collect_...
(AI_HYDRA_LEO_MISSING_A)
🔇 Additional comments (10)
nextflow.config (3)
1-1: LGTM: DSL2 enablement added correctly.Enabling DSL2 syntax is essential for the new workflow features introduced in this PR.
23-25: Good standardization of resource specifications.The explicit resource specifications (cpus, time, clusterOptions) provide better control and consistency for the Niagara cluster environment.
32-32: Process name correction appreciated.The correction from
registerNonLineartoregisterNonlinearfollows standard naming conventions.collect_and_combine_volumes.nf (1)
1-1: LGTM: DSL2 enablement consistent with other files.This change aligns with the DSL2 adoption across all workflow scripts in this PR.
collect_and_combine_volumes_niagara.nf (3)
1-2: LGTM: Clean modular approach using include statements.Good use of include statements to reuse functions from the main volume collection workflow.
13-17: LGTM: Proper handling of missing CSV files.The logic correctly handles missing CSV files by using a placeholder "NO_FILE" which is consistent with the collectVolumes process implementation.
8-21:✅ Verification successful
Verify regex pattern consistency with documentation.
The regex pattern
/_label_([\w]+)\.nii.gz/should be consistent with the pattern documented in README.md. Please ensure this matches the expected filename format.
🏁 Script executed:
#!/bin/bash # Description: Check if the regex pattern used here matches the one documented in README.md # Expected: Both patterns should be consistent echo "=== Checking regex patterns ===" echo "Pattern in collect_and_combine_volumes_niagara.nf:" rg "_label_\(.*\)\.nii\.gz" --type nf echo -e "\n=== Pattern mentioned in README.md ===" rg "_label_.*\.nii\.gz" README.mdLength of output: 3265
Regex pattern is consistent with documentation
The/_label_([\w]+)\.nii.gz/used in collect_and_combine_volumes_niagara.nf (lines 8–21) exactly matches the example in README.md. No updates are needed.magetbrain.nf (3)
1-1: LGTM! Enabling DSL 2 syntax.The addition of
nextflow.enable.dsl=2correctly enables Nextflow DSL 2 syntax, which is required for the workflow definitions in this file.
306-306: LGTM! Clean integration of the new workflow.The main workflow is cleanly updated to use the new modularized
collectAndCombineVolumesworkflow, improving code organization and maintainability.
248-277:✅ Verification successful
Verify downstream handling of placeholder files and consider memory optimization.
The new
collectAndCombineVolumesworkflow effectively modularizes the volume collection logic. However, there are a few considerations:
Placeholder file handling: Using
file("NO_FILE")as a placeholder (line 262) could cause issues if downstream processes (collectVolumes/combineVolumes) don't properly handle non-existent files.Memory usage: The
.collect()operation (line 275) will gather all volumes in memory before passing tocombineVolumes. This could be memory-intensive for large datasets.Please verify that the imported workflows can handle the
"NO_FILE"placeholder:
🏁 Script executed:
#!/bin/bash # Check if collectVolumes and combineVolumes workflows handle placeholder files echo "Checking for NO_FILE handling in collectVolumes and combineVolumes workflows..." rg -A 10 -B 5 "NO_FILE|file.*exists" ./collect_and_combine_volumes.nfLength of output: 815
NO_FILEplaceholder correctly handled; memory usage minimalDownstream scripts already guard against the
"NO_FILE"marker—inside collectVolumes the stub and real script both use:def labelCsvArg = labelCsv.name != 'NO_FILE' ? "${labelCsv}" : ""so no invalid path is ever passed to
collect_volumes_nifti.sh. And combineVolumes only consumes the generated TSVs, not the original CSVs, so it never sees"NO_FILE".As for the
.collect()call, it only gathers a list of file‐reference objects (not their contents), which is negligible in memory for all but truly massive channel sizes. If you anticipate millions of files, you could refactor to a streaming or chunked reduction, but for typical atlas‐sets this poses no concern.
1 participant
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores