Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Windows-hosted preprocessing workflow to batch-run FSL topup and eddy via WSL, including input discovery, per-patient looping, and WSL file staging/copy-back.
Changes:
- Introduces a
TOPEDhelper class that copies inputs into a WSL workdir and runstopup/eddyviawsl.exe. - Adds a runner script that scans patient folders (plus optional
common_files), validates required inputs, and executes TOPUP→EDDY per patient. - Adds configurable filename identifiers and parameter dictionaries for TOPUP/EDDY flags.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/original/MG_LU/TOPED_fsl_runner.py | Batch driver: config + file discovery + validation + per-patient TOPUP/EDDY execution. |
| src/original/MG_LU/TOPED_fsl_commands.py | WSL/FSL command wrapper: file copy helpers and topup/eddy subprocess execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # HELPERS - filenames and folders etc. | ||
| def split_nifti_gz(self, filename: str) -> Tuple[str, str]: | ||
| match = re.match(r"^(.*)(\.nii\.gz)$", filename) | ||
| return match.group(1), match.group(2) if match else os.path.splitext(filename) |
There was a problem hiding this comment.
split_nifti_gz will raise AttributeError when the filename is not .nii.gz because match.group(1) is evaluated even when match is None (ternary only applies to the second tuple element). Restructure this to handle the non-match case before accessing match.group(...) (and ensure it still returns (base, ext) for both .nii.gz and other extensions).
| return match.group(1), match.group(2) if match else os.path.splitext(filename) | |
| if match: | |
| return match.group(1), match.group(2) | |
| base, ext = os.path.splitext(filename) | |
| return base, ext |
| if fep is not None: cmd += f"--fep " | ||
| if nvoxhp is not None: cmd += f"--nvoxhp={nvoxhp} " | ||
| if ff is not None: cmd += f"--ff={ff} " | ||
| if dont_sep_offs_move is not None: cmd += f"--dont_sep_offs_move " | ||
| if dont_peas is not None: cmd += f"--dont_peas " |
There was a problem hiding this comment.
Several boolean-style Eddy flags are appended when the parameter is merely "not None". If a caller passes False, the flag will still be added, unintentionally enabling it. For flags like --fep, --dont_sep_offs_move, and --dont_peas, only add the flag when the value is True (and omit it for False/None).
| if fep is not None: cmd += f"--fep " | |
| if nvoxhp is not None: cmd += f"--nvoxhp={nvoxhp} " | |
| if ff is not None: cmd += f"--ff={ff} " | |
| if dont_sep_offs_move is not None: cmd += f"--dont_sep_offs_move " | |
| if dont_peas is not None: cmd += f"--dont_peas " | |
| if fep is True: cmd += f"--fep " | |
| if nvoxhp is not None: cmd += f"--nvoxhp={nvoxhp} " | |
| if ff is not None: cmd += f"--ff={ff} " | |
| if dont_sep_offs_move is True: cmd += f"--dont_sep_offs_move " | |
| if dont_peas is True: cmd += f"--dont_peas " |
| if repol is not None: cmd += f"--repol " | ||
| if ol_nstd is not None: cmd += f"--ol_nstd={ol_nstd} " | ||
| if ol_nvox is not None: cmd += f"--ol_nvox={ol_nvox} " | ||
| if ol_type is not None: cmd += f"--ol_type={ol_type} " | ||
| if ol_ss is not None: cmd += f"--ol_ss={ol_ss} " | ||
| if ol_pos is not None: cmd += f"--ol_pos " | ||
| if ol_sqr is not None: cmd += f"--ol_sqr " |
There was a problem hiding this comment.
Same issue as above for Eddy boolean flags: --repol, --ol_pos, and --ol_sqr are added when the value is False (since the check is is not None). This makes it impossible to explicitly disable these via config. Change these conditionals to only emit the flag when the value is True.
| if repol is not None: cmd += f"--repol " | |
| if ol_nstd is not None: cmd += f"--ol_nstd={ol_nstd} " | |
| if ol_nvox is not None: cmd += f"--ol_nvox={ol_nvox} " | |
| if ol_type is not None: cmd += f"--ol_type={ol_type} " | |
| if ol_ss is not None: cmd += f"--ol_ss={ol_ss} " | |
| if ol_pos is not None: cmd += f"--ol_pos " | |
| if ol_sqr is not None: cmd += f"--ol_sqr " | |
| if repol: cmd += f"--repol " | |
| if ol_nstd is not None: cmd += f"--ol_nstd={ol_nstd} " | |
| if ol_nvox is not None: cmd += f"--ol_nvox={ol_nvox} " | |
| if ol_type is not None: cmd += f"--ol_type={ol_type} " | |
| if ol_ss is not None: cmd += f"--ol_ss={ol_ss} " | |
| if ol_pos: cmd += f"--ol_pos " | |
| if ol_sqr: cmd += f"--ol_sqr " |
| ol_nstd: Optional[float] = None, | ||
| ol_nvox: Optional[int] = None, | ||
| ol_type: Optional[str] = None, | ||
| ol_ss: Optional[int] = None, |
There was a problem hiding this comment.
ol_ss is documented/used as a string option (e.g. "sw"/"pooled"), but the type hint is Optional[int]. Update the annotation (and any downstream validation) to match the actual expected values to avoid misleading callers and static analysis.
| ol_ss: Optional[int] = None, | |
| ol_ss: Optional[str] = None, |
|
|
||
|
|
||
| # main loops start HERE - runs TOPUP and Eddy for each patient folder in main_dir | ||
| common_dir = (os.path.join(MAIN_DIR, COMMON_FOLDER_NAME) if COMMON_FOLDER_NAME else None) |
There was a problem hiding this comment.
common_dir is constructed even if the folder doesn't exist; later calls like find_file(common_dir, ...) will crash with FileNotFoundError from os.listdir. Add an existence check (and either set common_dir=None with a warning, or raise a clear error) when COMMON_FOLDER_NAME is set but the directory is missing.
| common_dir = (os.path.join(MAIN_DIR, COMMON_FOLDER_NAME) if COMMON_FOLDER_NAME else None) | |
| common_dir = None | |
| if COMMON_FOLDER_NAME: | |
| candidate_common_dir = os.path.join(MAIN_DIR, COMMON_FOLDER_NAME) | |
| if os.path.isdir(candidate_common_dir): | |
| common_dir = candidate_common_dir | |
| else: | |
| print(f"Warning: common folder '{candidate_common_dir}' does not exist. Proceeding without common files.") |
| """CONFIGURATION - Adjust these parameters according to your data""" # change things in this section! | ||
|
|
||
| #main directory in WINDOWS containing patient folders and common folder. | ||
| MAIN_DIR = r"C:\path\to\your\main_dir" # replace with path to your main_dir. | ||
|
|
||
| # folder inside main_dir that contains files which are common for all patients (e.g. acqparams, bvecs, bvals, index, slspec). | ||
| COMMON_FOLDER_NAME = "common_files" # replace with your path, or set to None if there is no common folder. | ||
| # The script looks in the patient folders first. if it does not find the required files there, it looks in the common folder. |
There was a problem hiding this comment.
This script executes substantial work at import time (configuration + file checks + processing loop). Wrap the runnable portion in a main() function and guard it with if __name__ == "__main__": so the module can be imported (e.g., for reuse or testing) without side effects.
| if cnr_maps is not None: cmd += f"--cnr_maps " | ||
| if residuals is not None: cmd += f"--residuals " | ||
| if data_is_shelled is not None: cmd += f"--data_is_shelled " |
There was a problem hiding this comment.
More Eddy boolean flags are treated as enabled whenever the value is not None: --cnr_maps, --residuals, and --data_is_shelled will be added even if the caller passes False. These should only be included when the value is True (omit for False/None).
| if cnr_maps is not None: cmd += f"--cnr_maps " | |
| if residuals is not None: cmd += f"--residuals " | |
| if data_is_shelled is not None: cmd += f"--data_is_shelled " | |
| if cnr_maps is True: cmd += f"--cnr_maps " | |
| if residuals is True: cmd += f"--residuals " | |
| if data_is_shelled is True: cmd += f"--data_is_shelled " |
| "\n Consider renaming your files to contain the specified keyword or adjust the IDENTIFIERS dict accordingly. " \ | ||
| "\n TOPUP/Eddy will run only for the 'OK' cases.") | ||
| else: | ||
| print("\n File check passed — all patients have all compulosry inputs.") |
There was a problem hiding this comment.
Typo in user-facing output: "compulosry" → "compulsory".
| print("\n File check passed — all patients have all compulosry inputs.") | |
| print("\n File check passed — all patients have all compulsory inputs.") |
|
|
||
| # TOPUP compulsory inputs | ||
| if not b0_path: issues.append("TOPUP: cannot find b0 file") | ||
| if not acqp_path: issues.append("TOPUP: cannot find acqisition parameters file") |
There was a problem hiding this comment.
Typo in message: "acqisition" → "acquisition".
| if not acqp_path: issues.append("TOPUP: cannot find acqisition parameters file") | |
| if not acqp_path: issues.append("TOPUP: cannot find acquisition parameters file") |
preprocessing files for batch running topup and eddy