BIDS input#132
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a libBIDS git submodule and pinned commit, defaults two helper flags to "off", introduces a new Argbash CLI script to convert BIDS datasets to CSV (with optional mask output), and updates .gitignore to ignore Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as modelbuild_input_from_bids.sh
participant Lib as libBIDS.sh
participant FS as FileSystem
participant Out as OutputCSV
User->>CLI: invoke with options + args
CLI->>Lib: request BIDS rows (apply suffix/derivatives/column-filters)
Lib-->>CLI: return filtered rows
CLI->>FS: resolve file paths
CLI->>Out: write image paths to CSV
alt --masks provided
CLI->>Lib: request mask rows (suffix=mask)
Lib-->>CLI: return mask rows
CLI->>FS: resolve mask paths
CLI->>Out: write mask paths to masks file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🤖 Fix all issues with AI agents
In `@modelbuild_input_from_bids.sh`:
- Around line 164-166: The script uses a hardcoded developer-local path
(/home/gdevenyi/...) when sourcing libBIDS.sh which will fail for other users;
replace that line with the relative/submodule-based sourcing shown in the
commented line (use the BASH_SOURCE dirname approach: "$(cd "$(dirname
"${BASH_SOURCE[0]}")" && pwd)/libBIDS.sh/libBIDS.sh") and add a quick existence
check before sourcing (or a clear process exit error) so
modelbuild_input_from_bids.sh fails fast with a useful message if the submodule
file is missing.
- Around line 181-185: The call to libBIDSsh_csv_filter uses an unquoted array
expansion ${_arg_column_filter[*]} which can split elements with spaces; change
the expansion to a quoted form for the _arg_column_filter array (use
"${_arg_column_filter[@]}") when building filtered_image_table so each array
element is passed as a single argument to libBIDSsh_csv_filter.
- Around line 176-179: The current substitution on _arg_column_filter embeds the
flag and value into single elements, producing malformed arguments for
libBIDSsh_csv_filter; instead build a new array (e.g., _column_filter_args) by
iterating over _arg_column_filter and appending the flag and the value as
separate array entries (append "--row-filter" then the value for each), then
replace uses of the old combined string expansion with the new array expansion
("${_column_filter_args[@]}") when calling libBIDSsh_csv_filter (and at the
other call site referenced in the diff).
🧹 Nitpick comments (1)
modelbuild_input_from_bids.sh (1)
192-202: Same quoting issue and redundant redeclaration.
- Line 197 has the same unquoted array expansion issue.
- Line 198 redeclares
declare -A rowwhich is already declared on line 187—this is redundant.♻️ Proposed cleanup
-if [[ ! -z "${_arg_masks}" ]]; then +if [[ -n "${_arg_masks}" ]]; then filtered_mask_table=$(libBIDSsh_csv_filter "${bids_table}" \ -r "suffix:mask" \ -r "extension:(nii|nii.gz)" \ -r "derivatives:${_arg_derivatives}" \ - ${_arg_column_filter[*]}) - declare -A row + "${_column_filter_args[@]}") while libBIDS_csv_iterator "$filtered_mask_table" row "subject" "session"; do printf "%s\n" "${row[path]}" done >"${_arg_masks}" fi
There was a problem hiding this comment.
Pull request overview
Adds a new CLI helper script to generate modelbuild.sh inputs from a BIDS dataset, and wires in libBIDS.sh as a submodule; also hardens helper flag handling when env args are undefined.
Changes:
- Added
modelbuild_input_from_bids.shto parse BIDS datasets and filter/select images (and optional masks). - Added
libBIDS.shas a git submodule for BIDS parsing/filtering utilities. - Fixed
helpers.shlog-level selection to tolerate unset_arg_dry_run/_arg_debug.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modelbuild_input_from_bids.sh | New Argbash-based CLI script to parse/filter BIDS and write output path lists. |
| libBIDS.sh | Adds libBIDS.sh as a submodule pointer. |
| helpers.sh | Avoids unbound-variable errors when dry-run/debug flags aren’t defined. |
| .gitmodules | Registers the new libBIDS.sh submodule. |
| .gitignore | Ignores editor config and CSV outputs by default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| filtered_image_table=$(libBIDSsh_csv_filter "${bids_table}" \ | ||
| -r "suffix:${_arg_suffix}" \ | ||
| -r "extension:(nii|nii.gz)" \ | ||
| -r "derivatives:${_arg_derivatives}" \ | ||
| "${_column_filter_args[@]}") |
There was a problem hiding this comment.
The script defines --session and --run CLI options (and documents them in print_help), but they are never applied to the filter. As a result, those flags have no effect. Consider adding conditional -r \"session:${_arg_session}\" and -r \"run:${_arg_run}\" filters when the user supplies them (e.g., non-empty session and run != 'NA'), or folding them into _column_filter_args similarly to --column-filter.
| declare -A row | ||
| while libBIDS_csv_iterator "$filtered_image_table" row "subject" "session"; do | ||
| printf "%s\n" "${row[path]}" | ||
| done >"${_arg_output}" |
There was a problem hiding this comment.
The script mixes libBIDSsh_* function names (e.g., libBIDSsh_parse_bids_to_csv, libBIDSsh_csv_filter) with libBIDS_csv_iterator. If the iterator function in the submodule uses the same libBIDSsh_ prefix, this will fail at runtime with command not found. Align the iterator call to the actual exported function name from libBIDS.sh and keep the naming consistent within this script.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modelbuild_input_from_bids.sh`:
- Around line 180-184: The CLI options _arg_session and _arg_run are parsed but
not passed to libBIDSsh_csv_filter, so add session/run filters when building the
filter calls: update the filtered_image_table libBIDSsh_csv_filter invocation
and the mask filter invocation to include -r "session:${_arg_session}" and -r
"run:${_arg_run}" (only when those variables are non-empty) so that filtering
honors --session and --run; reference the existing calls that populate
filtered_image_table and the mask filter block that uses libBIDSsh_csv_filter to
apply the same conditional additions.
- Around line 174-178: The array expansions under set -u can raise "unbound
variable" on older Bash (<4.4); change usages of "${_arg_column_filter[@]}",
"${_column_filter_args[@]}", and any similar array expansions to the portable
idiom ${_arg_column_filter[@]+"${_arg_column_filter[@]}"} and
${_column_filter_args[@]+"${_column_filter_args[@]}"} so empty arrays don't
trigger an error while keeping the existing logic in the loop that builds
_column_filter_args and where that array is later expanded.
🧹 Nitpick comments (2)
modelbuild_input_from_bids.sh (2)
191-191: Prefer[[ -n ]]over[[ ! -z ]].Minor style nit:
[[ -n "${_arg_masks}" ]]is the more idiomatic equivalent of[[ ! -z "${_arg_masks}" ]].
162-164: Add ashellcheck sourcedirective forlibBIDS.shand consider a guard.Line 163 has
# shellcheck source=helpers.shbut line 164 is missing the corresponding directive. Also, if the submodule isn't initialized, this will fail with a confusing error. A quick existence check would improve the UX.🛡️ Proposed fix
# shellcheck source=helpers.sh source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/helpers.sh" +# shellcheck source=libBIDS.sh/libBIDS.sh +_libBIDS_path="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/libBIDS.sh/libBIDS.sh" +[[ -f "${_libBIDS_path}" ]] || die "libBIDS.sh not found — did you run 'git submodule update --init'?" 1 -source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/libBIDS.sh/libBIDS.sh" +source "${_libBIDS_path}"
| filtered_image_table=$(libBIDSsh_csv_filter "${bids_table}" \ | ||
| -r "suffix:${_arg_suffix}" \ | ||
| -r "extension:(nii|nii.gz)" \ | ||
| -r "derivatives:${_arg_derivatives}" \ | ||
| "${_column_filter_args[@]}") |
There was a problem hiding this comment.
--session and --run options are parsed but never applied as filters.
_arg_session and _arg_run are accepted via CLI (lines 73-88) but are not passed to libBIDSsh_csv_filter here or in the mask filter block (lines 192-196). Users specifying --session or --run will get no filtering effect, which is silently incorrect.
🐛 Proposed fix — add session/run filters when set
filtered_image_table=$(libBIDSsh_csv_filter "${bids_table}" \
-r "suffix:${_arg_suffix}" \
-r "extension:(nii|nii.gz)" \
-r "derivatives:${_arg_derivatives}" \
+ ${_arg_session:+-r "session:${_arg_session}"} \
+ ${_arg_run:+-r "run:${_arg_run}"} \
"${_column_filter_args[@]}")Apply the same addition to the mask filter block (lines 192-196).
🤖 Prompt for AI Agents
In `@modelbuild_input_from_bids.sh` around lines 180 - 184, The CLI options
_arg_session and _arg_run are parsed but not passed to libBIDSsh_csv_filter, so
add session/run filters when building the filter calls: update the
filtered_image_table libBIDSsh_csv_filter invocation and the mask filter
invocation to include -r "session:${_arg_session}" and -r "run:${_arg_run}"
(only when those variables are non-empty) so that filtering honors --session and
--run; reference the existing calls that populate filtered_image_table and the
mask filter block that uses libBIDSsh_csv_filter to apply the same conditional
additions.
gdevenyi
force-pushed
the
bids-input
branch
3 times, most recently
from
bbf3091 to
7014876
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modelbuild_input_from_bids.sh`:
- Line 176: The loop mistakenly calls libBIDS_csv_iterator instead of the
correct libBIDSsh_csv_iterator; update the while invocation that currently
references libBIDS_csv_iterator to call libBIDSsh_csv_iterator (the same helper
used earlier) so the loop over "$filtered_mask_table" with variables row,
subject, session uses the correct function and avoids the "command not found"
runtime error.
| -r "derivatives:${_arg_derivatives}" \ | ||
| "${_column_filter_args[@]}") | ||
| declare -A row | ||
| while libBIDS_csv_iterator "$filtered_mask_table" row "subject" "session"; do |
There was a problem hiding this comment.
Bug: libBIDS_csv_iterator should be libBIDSsh_csv_iterator.
Line 165 correctly calls libBIDSsh_csv_iterator, but line 176 drops the sh — calling libBIDS_csv_iterator instead. This will fail at runtime with "command not found" whenever --masks is used.
🐛 Proposed fix
- while libBIDS_csv_iterator "$filtered_mask_table" row "subject" "session"; do
+ while libBIDSsh_csv_iterator "$filtered_mask_table" row "subject" "session"; 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.
| while libBIDS_csv_iterator "$filtered_mask_table" row "subject" "session"; do | |
| while libBIDSsh_csv_iterator "$filtered_mask_table" row "subject" "session"; do |
🤖 Prompt for AI Agents
In `@modelbuild_input_from_bids.sh` at line 176, The loop mistakenly calls
libBIDS_csv_iterator instead of the correct libBIDSsh_csv_iterator; update the
while invocation that currently references libBIDS_csv_iterator to call
libBIDSsh_csv_iterator (the same helper used earlier) so the loop over
"$filtered_mask_table" with variables row, subject, session uses the correct
function and avoids the "command not found" runtime error.
Summary by CodeRabbit
New Features
Bug Fixes
Chores