Rename output suffixes to BIDS-compliant format#11
Conversation
Addresses issue #9: Rename final outputs according to BIDS specification. Changes: - Added bids_suffix() helper function for BIDS-compliant filename generation - Renamed main outputs to use BIDS entity-label pairs: - .denoise.mnc → _desc-denoised_T1w.mnc - .mask_withcsf.mnc → _label-brainwithcsf_dseg.mnc - .mask_nocsf.mnc → _label-brainnocsf_dseg.mnc - .classify.mnc → _dseg.mnc - Renamed transform files: - .affine_to_model.xfm → _from-model_to-affine.xfm - .nlin_to_model.xfm → _from-model_to-nlin.xfm - .nlin_from_model.xfm → _from-model_to-nlin_inverse.xfm - Renamed LSQ6 outputs with _space-LSQ6_ prefix - Renamed QC outputs with _desc-*_qc suffix - Removed non-existent posterior files from clobber checks See: https://bids-specification.readthedocs.io/en/stable/derivatives/introduction.html
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
synthstrip_N3.sh (1)
486-514:⚠️ Potential issue | 🟠 MajorBIDS entity labels must not contain underscores.
In BIDS filenames, underscores delimit
key-valueentity pairs. A suffix like_desc-bias_correction_qcwill be mis-parsed:desc=bias, thencorrectionandqcbecome orphan tokens without entity keys. The same issue affects_desc-mask-classified_qc.Use camelCase within labels instead:
Current Suggested _desc-bias_correction_qc_desc-biasCorrection_qc_desc-mask-classified_qc_desc-maskClassified_qc_desc-registration_qc_desc-registration_qc(OK)_desc-qc_desc-qc(OK)Also ensure
_qcis a recognized BIDS suffix or custom extension for your pipeline; otherwise it should be folded into thedesclabel (e.g.,_desc-biasCorrection_photo.jpgusing BIDS_photosuffix or a custom suffix).
🤖 Fix all issues with AI agents
In `@synthstrip_N3.sh`:
- Around line 590-610: The clobber check loop (guarded by _arg_clobber and
iterating over _arg_output plus multiple bids_suffix calls) omits the
bids_suffix call for the "_space-LSQ6" xfm file, so add $(bids_suffix
"_space-LSQ6" xfm) into the list inside the for loop alongside the other
bids_suffix entries (the same list that contains "_space-LSQ6_T1w" and other
"_space-LSQ6_*" items) so the _space-LSQ6.xfm file is checked with [[ -s
"${file}" ]] and triggers failure when --clobber is off.
- Around line 918-920: The three cp lines that copy to_model_*.xfm are using
reversed direction labels and transform-type names; rename the destination files
produced by bids_suffix so they use coordinate-space labels (e.g., replace
"_from-model_to-affine" and "_from-model_to-nlin" with
"_from-T1w_to-MNI152NLin2009cSym_mode-image_xfm" for the subject→MNI transforms
and use "_from-MNI152NLin2009cSym_to-T1w_mode-image_xfm" for the inverse
transform), i.e., update the cp targets for to_model_0_GenericAffine.xfm,
to_model_1_NL.xfm, and to_model_1_inverse_NL.xfm accordingly; also update the
clobber-check list entries that previously referenced the old names so they
match the new coordinate-space-based filenames.
🧹 Nitpick comments (3)
synthstrip_N3.sh (3)
347-353: Quote variables and split declaration from assignment inbids_suffix.Line 349 has two issues flagged by ShellCheck (SC2155, SC2046): the
localdeclaration masks the return value of the subshell, and${_arg_output}is unquoted insidedirname/basename, which will break on paths containing spaces. The same quoting concern applies to theechoon line 352.Proposed fix
function bids_suffix() { - local base=$(dirname ${_arg_output})/$(basename ${_arg_output} .mnc) - local suffix=$1 - local ext=${2:-mnc} - echo "${base}${suffix}.${ext}" + local base + base="$(dirname "${_arg_output}")/$(basename "${_arg_output}" .mnc)" + local suffix="$1" + local ext="${2:-mnc}" + echo "${base}${suffix}.${ext}" }
486-486: Quote all$(bids_suffix ...)expansions to prevent word splitting.Every call site uses unquoted
$(bids_suffix ...)(ShellCheck SC2046). If_arg_outputever contains spaces or glob characters, filenames will be split into multiple arguments. This applies to all ~25 call sites throughout the file (lines 486, 496, 506, 511–514, 592–606, 904–920, 928–948).Example fix for line 486:
- $(bids_suffix "_desc-bias_correction_qc" jpg) + "$(bids_suffix "_desc-bias_correction_qc" jpg)"
357-358: Duplicate comment.Line 358 is an exact duplicate of line 357.
# Create tmpdir in the form $TMPDIR/tmp.$SCRIPTNAME.$INPUTFILE -# Create tmpdir in the form $TMPDIR/tmp.$SCRIPTNAME.$INPUTFILE # Later functions extend to $TMPDIR/tmp.$SCRIPTNAME.$INPUTFILE/$FUNCTION
| if [[ "${_arg_clobber}" == "off" ]]; then | ||
| for file in ${_arg_output} \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).mask_nocsf.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).mask_withcsf.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).classify.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior1.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior2.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior3.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior4.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).denoise.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).affine_to_model.xfm \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).nlin_to_model.xfm \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).nlin_from_model.xfm \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).ICV.xfm \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.webp \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.mask.classified.jpg \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.bias.jpg \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.registration.jpg \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.mask_nocsf.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.mask_withcsf.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.classify.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior1.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior2.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior3.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior4.mnc \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.denoise.mnc; do | ||
| $(bids_suffix "_label-brainnocsf_dseg") \ | ||
| $(bids_suffix "_label-brainwithcsf_dseg") \ | ||
| $(bids_suffix "_dseg") \ | ||
| $(bids_suffix "_desc-denoised_T1w") \ | ||
| $(bids_suffix "_from-model_to-affine" xfm) \ | ||
| $(bids_suffix "_from-model_to-nlin" xfm) \ | ||
| $(bids_suffix "_from-model_to-nlin_inverse" xfm) \ | ||
| $(bids_suffix "_desc-qc" webp) \ | ||
| $(bids_suffix "_desc-mask-classified_qc" jpg) \ | ||
| $(bids_suffix "_desc-bias_correction_qc" jpg) \ | ||
| $(bids_suffix "_desc-registration_qc" jpg) \ | ||
| $(bids_suffix "_space-LSQ6_label-brainnocsf_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_label-brainwithcsf_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_T1w"); do | ||
| if [[ -s "${file}" ]]; then | ||
| failure "File ${file} already exists and --clobber not specified!" | ||
| fi | ||
| done | ||
| done |
There was a problem hiding this comment.
Missing _space-LSQ6.xfm from clobber check list.
Line 935 produces $(bids_suffix "_space-LSQ6" xfm), but this file is not included in the clobber check loop. A pre-existing file at that path would be silently overwritten even without --clobber.
Proposed fix
$(bids_suffix "_space-LSQ6_dseg") \
- $(bids_suffix "_space-LSQ6_T1w"); do
+ $(bids_suffix "_space-LSQ6_T1w") \
+ $(bids_suffix "_space-LSQ6" xfm); do📝 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.
| if [[ "${_arg_clobber}" == "off" ]]; then | |
| for file in ${_arg_output} \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).mask_nocsf.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).mask_withcsf.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).classify.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior1.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior2.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior3.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).posterior4.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).denoise.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).affine_to_model.xfm \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).nlin_to_model.xfm \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).nlin_from_model.xfm \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).ICV.xfm \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.webp \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.mask.classified.jpg \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.bias.jpg \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.registration.jpg \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.mask_nocsf.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.mask_withcsf.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.classify.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior1.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior2.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior3.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.posterior4.mnc \ | |
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.denoise.mnc; do | |
| $(bids_suffix "_label-brainnocsf_dseg") \ | |
| $(bids_suffix "_label-brainwithcsf_dseg") \ | |
| $(bids_suffix "_dseg") \ | |
| $(bids_suffix "_desc-denoised_T1w") \ | |
| $(bids_suffix "_from-model_to-affine" xfm) \ | |
| $(bids_suffix "_from-model_to-nlin" xfm) \ | |
| $(bids_suffix "_from-model_to-nlin_inverse" xfm) \ | |
| $(bids_suffix "_desc-qc" webp) \ | |
| $(bids_suffix "_desc-mask-classified_qc" jpg) \ | |
| $(bids_suffix "_desc-bias_correction_qc" jpg) \ | |
| $(bids_suffix "_desc-registration_qc" jpg) \ | |
| $(bids_suffix "_space-LSQ6_label-brainnocsf_dseg") \ | |
| $(bids_suffix "_space-LSQ6_label-brainwithcsf_dseg") \ | |
| $(bids_suffix "_space-LSQ6_dseg") \ | |
| $(bids_suffix "_space-LSQ6_T1w"); do | |
| if [[ -s "${file}" ]]; then | |
| failure "File ${file} already exists and --clobber not specified!" | |
| fi | |
| done | |
| done | |
| if [[ "${_arg_clobber}" == "off" ]]; then | |
| for file in ${_arg_output} \ | |
| $(bids_suffix "_label-brainnocsf_dseg") \ | |
| $(bids_suffix "_label-brainwithcsf_dseg") \ | |
| $(bids_suffix "_dseg") \ | |
| $(bids_suffix "_desc-denoised_T1w") \ | |
| $(bids_suffix "_from-model_to-affine" xfm) \ | |
| $(bids_suffix "_from-model_to-nlin" xfm) \ | |
| $(bids_suffix "_from-model_to-nlin_inverse" xfm) \ | |
| $(bids_suffix "_desc-qc" webp) \ | |
| $(bids_suffix "_desc-mask-classified_qc" jpg) \ | |
| $(bids_suffix "_desc-bias_correction_qc" jpg) \ | |
| $(bids_suffix "_desc-registration_qc" jpg) \ | |
| $(bids_suffix "_space-LSQ6_label-brainnocsf_dseg") \ | |
| $(bids_suffix "_space-LSQ6_label-brainwithcsf_dseg") \ | |
| $(bids_suffix "_space-LSQ6_dseg") \ | |
| $(bids_suffix "_space-LSQ6_T1w") \ | |
| $(bids_suffix "_space-LSQ6" xfm); do | |
| if [[ -s "${file}" ]]; then | |
| failure "File ${file} already exists and --clobber not specified!" | |
| fi | |
| done |
🤖 Prompt for AI Agents
In `@synthstrip_N3.sh` around lines 590 - 610, The clobber check loop (guarded by
_arg_clobber and iterating over _arg_output plus multiple bids_suffix calls)
omits the bids_suffix call for the "_space-LSQ6" xfm file, so add $(bids_suffix
"_space-LSQ6" xfm) into the list inside the for loop alongside the other
bids_suffix entries (the same list that contains "_space-LSQ6_T1w" and other
"_space-LSQ6_*" items) so the _space-LSQ6.xfm file is checked with [[ -s
"${file}" ]] and triggers failure when --clobber is off.
| cp -f ${tmpdir}/to_model_0_GenericAffine.xfm $(bids_suffix "_from-model_to-affine" xfm) | ||
| cp -f ${tmpdir}/to_model_1_NL.xfm $(bids_suffix "_from-model_to-nlin" xfm) | ||
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_inverse" xfm) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and look at the relevant lines
head -950 synthstrip_N3.sh | tail -50Repository: CoBrALab/synthstrip_N3
Length of output: 2617
🏁 Script executed:
# Also check the clobber-check list mentioned at lines 596-598
sed -n '590,605p' synthstrip_N3.shRepository: CoBrALab/synthstrip_N3
Length of output: 800
🌐 Web query:
BIDS derivatives specification from to entities coordinate space transforms
💡 Result:
In the current BIDS Derivatives spec, spatial coordinate spaces for derivative images are primarily expressed with:
space-<label>entity in the filename: indicates the coordinate system / spatial reference the file is interpreted in (for imaging, effectively “aligned to this reference”). The spec points you to the Coordinate Systems appendix for common space labels. [1]SpatialReferencemetadata in the JSON sidecar: required whenspaceis omitted or when thespacelabel is not one of the standard template identifiers; it can beorig(per-image unique space) or a path/URI to the reference image/file. [2]
Where do from- / to- fit in?
BIDS includes examples of transform filenames using from- and to-, e.g.
sub-001_from-T1w_to-MNI305_mode-image_xfm.h5, and explicitly warns that the transform-file specification is not finalized (“BEP 014 … under development”) and may change. [3][4]
Because of that:
from-/to-are not listed among the stable, standard entities in the main Entities appendix (which does includespace,desc, etc.). This reflects their “not fully standardized yet” status. [1][5]- Community guidance similarly notes that transform naming is currently unspecified in BIDS, pending BEP 014. [6]
Practical takeaway for “coordinate space ↔ transform” bookkeeping
- Use
space-...+SpatialReferenceto state what space an output is in and what reference defines it. [2][1] - If you distribute transforms, the commonly used
from-<source>_to-<target>_mode-<...>_xfm.<ext>pattern is consistent with current spec examples, but treat it as provisional until BEP 014 is finalized. [3][4][6]
Sources:
[1] BIDS Entities (stable) — space-<label> definition and entity list: (bids-specification.readthedocs.io)
[2] BIDS Derivatives “Common data types and metadata” — SpatialReference, orig: (bids-standard.github.io)
[3] BIDS Derivatives “Imaging data types” — from-..._to-..._mode-..._xfm... example + BEP 014 warning: (bids-specification.readthedocs.io)
[4] BIDS “Templates and atlases” (dev) — similar xfm example + BEP 014 warning: (bids-specification.readthedocs.io)
[5] Confirms stable entity list does not include from/to: (bids-specification.readthedocs.io)
[6] Neurostars discussion: transforms currently unspecified; BEP 014 exists: (neurostars.org)
Citations:
- 1: https://bids-specification.readthedocs.io/en/stable/appendices/entities.html
- 2: https://bids-standard.github.io/bids-specification-ignore/src/05-derivatives/02-common-data-types.html
- 3: https://bids-specification.readthedocs.io/en/stable/derivatives/imaging.html
- 4: https://bids-specification.readthedocs.io/en/latest/derivatives/atlas.html?utm_source=openai
- 5: https://bids-specification.readthedocs.io/en/stable/appendices/entities.html
- 6: https://neurostars.org/t/naming-convention-for-transformation-files-according-to-bids-specifications/20475?utm_source=openai
🏁 Script executed:
# Search for model/atlas references
grep -n "model\|atlas\|MNI\|template" synthstrip_N3.sh | head -30Repository: CoBrALab/synthstrip_N3
Length of output: 2355
🏁 Script executed:
# Look for variable definitions related to model/space
grep -n "RESAMPLE\|MODEL\|model_" synthstrip_N3.sh | head -20Repository: CoBrALab/synthstrip_N3
Length of output: 1918
🏁 Script executed:
# Check if there's any documentation or comments about what the transforms do
sed -n '1,100p' synthstrip_N3.shRepository: CoBrALab/synthstrip_N3
Length of output: 4929
Transform from-/to- direction is reversed; coordinate spaces should replace transform types.
The transforms map subject → model, but the current naming (from-model_to-affine) has the direction backwards. The files should use coordinate space labels instead of transform type descriptors:
to_model_0_GenericAffine.xfmmaps subject → MNI152NLin2009cSym (affine) →from-T1w_to-MNI152NLin2009cSym_mode-image_xfmto_model_1_NL.xfmmaps subject → MNI152NLin2009cSym (nonlinear) →from-T1w_to-MNI152NLin2009cSym_mode-image_xfm(with nonlinear mode specification)to_model_1_inverse_NL.xfmmaps MNI152NLin2009cSym → subject →from-MNI152NLin2009cSym_to-T1w_mode-image_xfm
Update the clobber-check list (lines 596–598) to match. Note: BIDS transform file naming (BEP 014) remains under development and is not yet finalized; the approach above aligns with current provisional guidance in the spec examples.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 918-918: Quote this to prevent word splitting.
(SC2046)
[warning] 919-919: Quote this to prevent word splitting.
(SC2046)
[warning] 920-920: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In `@synthstrip_N3.sh` around lines 918 - 920, The three cp lines that copy
to_model_*.xfm are using reversed direction labels and transform-type names;
rename the destination files produced by bids_suffix so they use
coordinate-space labels (e.g., replace "_from-model_to-affine" and
"_from-model_to-nlin" with "_from-T1w_to-MNI152NLin2009cSym_mode-image_xfm" for
the subject→MNI transforms and use
"_from-MNI152NLin2009cSym_to-T1w_mode-image_xfm" for the inverse transform),
i.e., update the cp targets for to_model_0_GenericAffine.xfm, to_model_1_NL.xfm,
and to_model_1_inverse_NL.xfm accordingly; also update the clobber-check list
entries that previously referenced the old names so they match the new
coordinate-space-based filenames.
There was a problem hiding this comment.
Pull request overview
This pull request implements BIDS-compliant naming conventions for all output files generated by the synthstrip_N3.sh script, addressing issue #9. The changes introduce a helper function to generate BIDS entity-label pair formatted filenames and systematically rename all outputs including brain masks, classifications, transforms, LSQ6 resampled outputs, and QC images.
Changes:
- Added
bids_suffix()helper function to generate BIDS-compliant filenames from base output path - Renamed main outputs (denoised images, masks, classifications) to use BIDS entity-label format with descriptive suffixes
- Renamed transform files with BIDS-style directional naming and LSQ6 space indicators
- Updated clobber checks to use new BIDS-compliant filenames and removed references to non-existent posterior files
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ${tmpdir}/qc/t_orig_spect.rgb \ | ||
| ${tmpdir}/qc/t_corrected_spect.rgb \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.bias.jpg | ||
| $(bids_suffix "_desc-bias_correction_qc" jpg) |
There was a problem hiding this comment.
The BIDS suffix "_desc-bias_correction_qc" contains an underscore within the description value ("bias_correction"), which is not compliant with BIDS naming conventions. According to BIDS, entity values (the part after the hyphen) should not contain underscores as they are reserved for separating entity-key pairs. The description should use a different separator like a dash or concatenation. Consider using "_desc-biascorrection_qc" or "_desc-bias-correction_qc" instead.
|
|
||
| # Helper function to add BIDS-compliant suffix to basename | ||
| function bids_suffix() { | ||
| local base=$(dirname ${_arg_output})/$(basename ${_arg_output} .mnc) |
There was a problem hiding this comment.
The bids_suffix function does not properly quote variable expansions, which could cause issues if the output path contains spaces or special characters. The variables should be quoted:
- Line 349:
local base="$(dirname "${_arg_output}")/$(basename "${_arg_output}" .mnc)" - Line 352:
echo "${base}${suffix}.${ext}"
This is a common bash best practice to prevent word splitting and glob expansion issues.
| local base=$(dirname ${_arg_output})/$(basename ${_arg_output} .mnc) | |
| local base | |
| base="$(dirname -- "${_arg_output}")/$(basename -- "${_arg_output}" .mnc)" |
| $(bids_suffix "_space-LSQ6_label-brainnocsf_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_label-brainwithcsf_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_dseg") \ | ||
| $(bids_suffix "_space-LSQ6_T1w"); do |
There was a problem hiding this comment.
The clobber check is missing the LSQ6 transform file that is created on line 935. The file $(bids_suffix "_space-LSQ6" xfm) should be added to the clobber check list to ensure consistency with other outputs.
| $(bids_suffix "_space-LSQ6_T1w"); do | |
| $(bids_suffix "_space-LSQ6_T1w") \ | |
| $(bids_suffix "_space-LSQ6" xfm); do |
| fi | ||
|
|
||
| cp -f ${tmpdir}/lsq6.xfm $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).lsq6.xfm | ||
| cp -f ${tmpdir}/lsq6.xfm $(bids_suffix "_space-LSQ6" xfm) |
There was a problem hiding this comment.
The transform file naming on line 935 is incomplete. The file $(bids_suffix "_space-LSQ6" xfm) only has a space entity but no indication of what kind of transform it is or its direction. According to BIDS conventions, transforms should indicate their source and target spaces. This should probably be something like _from-native_to-LSQ6_mode-rigid or similar to clearly indicate the transformation direction and type.
| cp -f ${tmpdir}/lsq6.xfm $(bids_suffix "_space-LSQ6" xfm) | |
| cp -f ${tmpdir}/lsq6.xfm $(bids_suffix "_from-native_to-LSQ6_mode-rigid" xfm) |
| failure "File ${file} already exists and --clobber not specified!" | ||
| fi | ||
| done | ||
| done |
There was a problem hiding this comment.
The done keyword on line 610 should be indented to match the for loop on line 591. The proper indentation would be two spaces to align with the for statement, maintaining consistency with bash coding conventions.
| done | |
| done |
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).nlin_from_model.xfm | ||
| cp -f ${tmpdir}/to_model_0_GenericAffine.xfm $(bids_suffix "_from-model_to-affine" xfm) | ||
| cp -f ${tmpdir}/to_model_1_NL.xfm $(bids_suffix "_from-model_to-nlin" xfm) | ||
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_inverse" xfm) |
There was a problem hiding this comment.
The suffix "_from-model_to-nlin_inverse" on line 920 appears to mix entity naming with a suffix modifier. According to BIDS conventions, "inverse" should probably be indicated using a mode entity (e.g., _from-model_to-native_mode-inverse) or handled differently. The current naming is ambiguous as it's unclear whether "inverse" is part of the "to" entity value or a separate modifier.
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_inverse" xfm) | |
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_mode-inverse" xfm) |
| cp -f ${tmpdir}/to_model_0_GenericAffine.xfm $(bids_suffix "_from-model_to-affine" xfm) | ||
| cp -f ${tmpdir}/to_model_1_NL.xfm $(bids_suffix "_from-model_to-nlin" xfm) | ||
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_inverse" xfm) |
There was a problem hiding this comment.
The transform naming convention appears to be inconsistent with typical BIDS conventions. According to BIDS, the "from" and "to" entities indicate the direction of the transform. The current naming "_from-model_to-affine" suggests the transform goes from model space to affine space, but the original file names (e.g., "to_model_0_GenericAffine.xfm") suggest these are transforms TO the model space, not FROM it.
The naming should more likely be:
- "_from-native_to-model_mode-affine.xfm" (or "_from-subject_to-model_mode-affine.xfm")
- "_from-native_to-model_mode-nonlinear.xfm"
- "_from-model_to-native_mode-nonlinear.xfm" (for the inverse)
This would correctly indicate that the forward transforms go from native/subject space TO model space, and the inverse goes from model TO native space.
| cp -f ${tmpdir}/to_model_0_GenericAffine.xfm $(bids_suffix "_from-model_to-affine" xfm) | |
| cp -f ${tmpdir}/to_model_1_NL.xfm $(bids_suffix "_from-model_to-nlin" xfm) | |
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-nlin_inverse" xfm) | |
| cp -f ${tmpdir}/to_model_0_GenericAffine.xfm $(bids_suffix "_from-native_to-model_mode-affine" xfm) | |
| cp -f ${tmpdir}/to_model_1_NL.xfm $(bids_suffix "_from-native_to-model_mode-nonlinear" xfm) | |
| cp -f ${tmpdir}/to_model_1_inverse_NL.xfm $(bids_suffix "_from-model_to-native_mode-nonlinear" xfm) |
| ${tmpdir}/qc/t_mask.rgb \ | ||
| ${tmpdir}/qc/t_classified.rgb \ | ||
| $(dirname ${_arg_output})/$(basename ${_arg_output} .mnc).qc.mask.classified.jpg | ||
| $(bids_suffix "_desc-mask-classified_qc" jpg) |
There was a problem hiding this comment.
The BIDS suffix "_desc-mask-classified_qc" uses a dash within the description value ("mask-classified"), which may be ambiguous. While dashes are sometimes used within entity values in BIDS, this could be confused with separate entities. Consider using "_desc-maskclassified_qc" for clarity and to follow common BIDS practices where entity values don't contain special separators.
| # Create tmpdir in the form $TMPDIR/tmp.$SCRIPTNAME.$INPUTFILE | ||
| # Create tmpdir in the form $TMPDIR/tmp.$SCRIPTNAME.$INPUTFILE |
There was a problem hiding this comment.
This comment appears twice on consecutive lines (357 and 358), which seems unintentional. One of these duplicate lines should be removed.
Addresses issue #9: Rename final outputs according to BIDS specification.
Changes:
See: https://bids-specification.readthedocs.io/en/stable/derivatives/introduction.html
Summary by CodeRabbit