Switch to jacobians for affine computed from antsTransformInfo #125
Conversation
WalkthroughRefactors dbm.sh to remove warp-field generation, compute Jacobian determinants from transforms via antsTransformInfo into text files, and adjust final Jacobian construction to incorporate scalar determinants. Renames delin/warp workflows to delin/affine, simplifies directories and dependencies, updates geometric flag handling, adds shell safety, and reformats Argbash blocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User/Caller
participant S as dbm.sh
participant A as antsTransformInfo
participant FS as File System
participant IM as ImageMath
participant D as Downstream Steps
U->>S: Run DBM post-processing
Note over S: Delin/affine workflow (no warp fields)
S->>A: Query transform metadata
A-->>S: Determinant value(s)
S->>FS: Write determinant txt (e.g., .../jacobian/*.txt)
S->>IM: Build final jacobian image<br/>using nlin image + scalar determinant
IM-->>S: Jacobian image
S->>D: Proceed with gen_rel_jacobian / gen_full_jacobian
Note over S,D: Dependencies updated to affine-based chain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbm.sh (1)
531-534: Conditionalize dependency on resampling job.When --target-space != final-target, the resample job isn’t submitted; this unconditional dependency can stall smoothing.
- qbatch ${_arg_block} --logdir ${_arg_output_dir}/dbm/logs/${__datetime} \ - --walltime ${_arg_walltime} \ - -N ${_arg_jobname_prefix}dbm_${__datetime}_smooth_jacobian \ - --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_full_jacobian \ - --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_rel_jacobian \ - --depend ${_arg_jobname_prefix}dbm_${__datetime}_resample_jacobians_final_target \ - ${_arg_output_dir}/dbm/jobs/${__datetime}/smooth_jacobian + if [[ ${_arg_target_space} == "final-target" ]]; then + qbatch ${_arg_block} --logdir ${_arg_output_dir}/dbm/logs/${__datetime} \ + --walltime ${_arg_walltime} \ + -N ${_arg_jobname_prefix}dbm_${__datetime}_smooth_jacobian \ + --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_full_jacobian \ + --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_rel_jacobian \ + --depend ${_arg_jobname_prefix}dbm_${__datetime}_resample_jacobians_final_target \ + ${_arg_output_dir}/dbm/jobs/${__datetime}/smooth_jacobian + else + qbatch ${_arg_block} --logdir ${_arg_output_dir}/dbm/logs/${__datetime} \ + --walltime ${_arg_walltime} \ + -N ${_arg_jobname_prefix}dbm_${__datetime}_smooth_jacobian \ + --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_full_jacobian \ + --depend ${_arg_jobname_prefix}dbm_${__datetime}_gen_rel_jacobian \ + ${_arg_output_dir}/dbm/jobs/${__datetime}/smooth_jacobian + fi
🧹 Nitpick comments (4)
dbm.sh (4)
314-320: Make determinant parsing robust and portable.antsTransformInfo output may include a colon; $2 could be “:”. Also requires gawk for log(). Prefer a field separator and last field, and quote paths.
- echo "antsTransformInfo ${_arg_output_dir}/final/transforms/$(basename ${file} | extension_strip)_0GenericAffine.mat \ - | grep Determinant \ - | awk '{print log(\$2)}' \ - > ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).txt" + echo "antsTransformInfo '${_arg_output_dir}/final/transforms/$(basename "${file}" | extension_strip)_0GenericAffine.mat' \ + | awk -F '[: ]+' '/Determinant/ {print log(\$NF)}' \ + > '${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename "${file}" | extension_strip).txt'"Please confirm antsTransformInfo prints a “Determinant” line and that gawk is available on your clusters. If not, we can compute det(log) from the 3x3 matrix parameters directly.
377-385: Same determinant parsing concerns for delin affine.Mirror the robust parsing and quoting.
- echo "antsTransformInfo ${_arg_output_dir}/dbm/intermediate/delin/affine/$(basename ${file} | extension_strip).mat \ - | grep Determinant \ - | awk '{print log(\$2)}' \ - > ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).txt" + echo "antsTransformInfo '${_arg_output_dir}/dbm/intermediate/delin/affine/$(basename "${file}" | extension_strip).mat' \ + | awk -F '[: ]+' '/Determinant/ {print log(\$NF)}' \ + > '${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename "${file}" | extension_strip).txt'"
274-278: Minor: remove stray double slash in mkdir path.No behavioral change, just cleanliness.
-mkdir -p ${_arg_output_dir}/dbm/jacobian/{final-target,}/{relative,full}//smooth +mkdir -p ${_arg_output_dir}/dbm/jacobian/{final-target,}/{relative,full}/smooth
49-49: Default for _arg_inputs can cause early “missing input” noise.Empty string as default hits the -s check. Consider leaving the array empty and failing fast if no positional provided.
-_arg_inputs=('') +_arg_inputs=()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbm.sh(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dbm.sh (4)
commonspace_resample.sh (2)
die(27-33)spacegroup(48-56)subjectspace_resample.sh (2)
die(29-35)spacegroup(50-58)twolevel_dbm.sh (2)
die(22-28)spacegroup(32-40)helpers.sh (3)
info(84-87)extension_strip(126-128)debug(88-91)
🪛 Shellcheck (0.10.0)
dbm.sh
[warning] 404-404: Quote this to prevent word splitting.
(SC2046)
[warning] 425-425: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (2)
dbm.sh (2)
280-284: LGTM: geometric flag normalized to 0/1 for ANTs.Clear, consistent with CreateJacobianDeterminantImage expectations.
213-215: LGTM: safer shell options.set -uo pipefail and -eE are appropriate here.
| -N ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \ | ||
| --depend ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \ | ||
| ${_arg_output_dir}/dbm/jobs/${__datetime}/jacobian_from_delin_affine | ||
| fi |
There was a problem hiding this comment.
Self-dependency deadlock: job depends on itself.
This prevents the job from ever running. It should depend on the delin affine-estimation job.
- -N ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \
- --depend ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \
- ${_arg_output_dir}/dbm/jobs/${__datetime}/jacobian_from_delin_affine
+ -N ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \
+ --depend ${_arg_jobname_prefix}dbm_${__datetime}_delin_affine_from_warp \
+ ${_arg_output_dir}/dbm/jobs/${__datetime}/jacobian_from_delin_affine📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -N ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \ | |
| --depend ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \ | |
| ${_arg_output_dir}/dbm/jobs/${__datetime}/jacobian_from_delin_affine | |
| fi | |
| -N ${_arg_jobname_prefix}dbm_${__datetime}_jacobian_from_delin_affine \ | |
| --depend ${_arg_jobname_prefix}dbm_${__datetime}_delin_affine_from_warp \ | |
| ${_arg_output_dir}/dbm/jobs/${__datetime}/jacobian_from_delin_affine | |
| fi |
🤖 Prompt for AI Agents
In dbm.sh around lines 392-395, the job is currently set to depend on its own
name (self-dependency), causing a deadlock; change the dependency from the
jacobian_from_delin_affine job name to the delin affine-estimation job name used
earlier in the script (i.e., replace the --depend value that references
dbm_${__datetime}_jacobian_from_delin_affine with the correct job name for the
delin affine-estimation job and ensure the -N naming remains the jacobian job as
intended so the jacobian job depends on the completion of the delin
affine-estimation job).
| echo "ImageMath 3 \ | ||
| ${_arg_output_dir}/dbm/jacobian/full/$(basename ${file} | extension_strip).nii.gz \ | ||
| + ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).nii.gz \ | ||
| ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz" | ||
| + ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | ||
| $(cat ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).txt)" | ||
| fi |
There was a problem hiding this comment.
Fix TOCTOU: defer reading scalar determinant until job execution (also quote to satisfy ShellCheck).
$(cat ...) is expanded now when generating the job file, racing with the upstream job that creates the .txt and tripping SC2046. Defer evaluation inside the job by escaping the $ and quote paths/values.
Apply:
- echo "ImageMath 3 \
- ${_arg_output_dir}/dbm/jacobian/full/$(basename ${file} | extension_strip).nii.gz \
- + ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \
- $(cat ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).txt)"
+ echo "ImageMath 3 \
+ '${_arg_output_dir}/dbm/jacobian/full/$(basename "${file}" | extension_strip).nii.gz' \
+ + '${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename "${file}" | extension_strip).nii.gz' \
+ \$(cat '${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename "${file}" | extension_strip).txt')"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "ImageMath 3 \ | |
| ${_arg_output_dir}/dbm/jacobian/full/$(basename ${file} | extension_strip).nii.gz \ | |
| + ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).nii.gz \ | |
| ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz" | |
| + ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | |
| $(cat ${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} | extension_strip).txt)" | |
| fi | |
| echo "ImageMath 3 \ | |
| '${_arg_output_dir}/dbm/jacobian/full/$(basename "${file}" | extension_strip).nii.gz' \ | |
| '${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename "${file}" | extension_strip).nii.gz' \ | |
| \$(cat '${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename "${file}" | extension_strip).txt')" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 404-404: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In dbm.sh around lines 401–405, the job file currently expands $(cat
${_arg_output_dir}/dbm/intermediate/affine/jacobian/$(basename ${file} |
extension_strip).txt) at generation time causing a TOCTOU race and ShellCheck
SC2046 warnings; change it so the cat is evaluated inside the generated job by
escaping the $ (e.g. use \$(cat ...)) and quote all path and variable expansions
(wrap _arg_output_dir, file, and any command substitutions in double quotes) so
the scalar determinant is read at job execution time and ShellCheck SC2046 is
satisfied.
| ${_arg_output_dir}/dbm/jacobian/relative/$(basename ${file} | extension_strip).nii.gz \ | ||
| + ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | ||
| ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz" | ||
| - ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | ||
| $(cat ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).txt)" | ||
| fi |
There was a problem hiding this comment.
Same TOCTOU/quoting issue for relative Jacobian.
Defer the subshell and quote paths/values.
- echo "ImageMath 3 \
- ${_arg_output_dir}/dbm/jacobian/relative/$(basename ${file} | extension_strip).nii.gz \
- - ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \
- $(cat ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).txt)"
+ echo "ImageMath 3 \
+ '${_arg_output_dir}/dbm/jacobian/relative/$(basename "${file}" | extension_strip).nii.gz' \
+ - '${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename "${file}" | extension_strip).nii.gz' \
+ \$(cat '${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename "${file}" | extension_strip).txt')"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${_arg_output_dir}/dbm/jacobian/relative/$(basename ${file} | extension_strip).nii.gz \ | |
| + ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | |
| ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz" | |
| - ${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename ${file} | extension_strip).nii.gz \ | |
| $(cat ${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename ${file} | extension_strip).txt)" | |
| fi | |
| echo "ImageMath 3 \ | |
| '${_arg_output_dir}/dbm/jacobian/relative/$(basename \"${file}\" | extension_strip).nii.gz' \ | |
| - '${_arg_output_dir}/dbm/intermediate/nlin/jacobian/$(basename \"${file}\" | extension_strip).nii.gz' \ | |
| \$(cat '${_arg_output_dir}/dbm/intermediate/delin/jacobian/$(basename \"${file}\" | extension_strip).txt')" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 425-425: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In dbm.sh around lines 423 to 426, the construction for the relative Jacobian
uses immediate subshell evaluations and unquoted paths which introduces TOCTOU
and word-splitting issues; modify the code to defer the subshell evaluation
(assign the basename/extension-stripped filename and the cat output to variables
first) and then use those variables in the command while quoting all path and
variable expansions (e.g. quote ${_arg_output_dir}, the computed filename
variable, and the contents read from the text file) so no unquoted substitutions
or subshells occur inline.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores