Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures the WisecondorX codebase by consolidating stage logic into fewer modules, adds a refqc CLI subcommand for reference QC, and updates the project to require Python 3.12+.
Changes:
- Consolidated convert/newref/predict logic into
src/wisecondorx/{convert,newref,predict}.py, removing older*_tools.py/*_control.pymodules. - Added
refqcCLI subcommand and expanded QC-related tests. - Added/updated unit tests for convert, newref, predict, and CLI version behavior; bumped Python requirement to >= 3.12.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ref_qc.py | Updates imports and adds unit tests for new refqc internals. |
| tests/test_predict_contract.py | Updates contract tests to import from consolidated predict.py. |
| tests/test_predict.py | Adds new functional tests covering consolidated predict utilities. |
| tests/test_newref_contract.py | Updates contract tests to import from consolidated newref.py. |
| tests/test_newref.py | Adds new functional tests covering consolidated newref utilities and tools. |
| tests/test_main.py | Adds CLI test for --version/-v. |
| tests/test_convert.py | Adds functional tests for consolidated convert implementation using pysam. |
| src/wisecondorx/utils.py | Moves shared helpers here and renames gender_correct to sex_correct; modernizes some strings/docstrings. |
| src/wisecondorx/refqc.py | Adds Typer CLI wrapper function (wcx_refqc) and refactors/optimizes parts of QC computations. |
| src/wisecondorx/predict_tools.py | Deleted (logic moved into predict.py). |
| src/wisecondorx/predict_output.py | Deleted (logic moved into predict.py). |
| src/wisecondorx/predict_control.py | Deleted (logic moved into predict.py). |
| src/wisecondorx/predict.py | New consolidated predict module including CLI commands and supporting functions. |
| src/wisecondorx/newref_tools.py | Deleted (logic moved into newref.py). |
| src/wisecondorx/newref_control.py | Deleted (logic moved into newref.py). |
| src/wisecondorx/newref.py | New consolidated newref module including CLI command and supporting functions; calls refqc after building. |
| src/wisecondorx/main.py | Simplifies CLI wiring to consolidated modules; adds refqc command and version flag. |
| src/wisecondorx/convert_tools.py | Deleted (logic moved into convert.py). |
| src/wisecondorx/convert.py | New consolidated convert module. |
| pyproject.toml | Bumps requires-python and Ruff target version to 3.12. |
| pixi.lock | Updates locked metadata for Python requirement. |
| docs/include/pipeline/predict.sh | Deleted legacy pipeline helper script. |
| docs/include/pipeline/newref.sh | Deleted legacy pipeline helper script. |
| docs/include/pipeline/convert.sh | Deleted legacy pipeline helper script. |
| README.md | Removes references to deleted pipeline scripts. |
| AGENTS.md | Updates project architecture and stack description to reflect restructuring and Typer CLI. |
Comments suppressed due to low confidence (1)
src/wisecondorx/refqc.py:257
- As a Typer subcommand, the integer returned by
wcx_refqcwon’t propagate to the process exit code, sowisecondorx refqc ...will exit 0 even when QC fails. If QC status is meant for automation/pipelines, raisetyper.Exit(code=worst)(after writing JSON/logging) instead of only returning the integer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gender = predict_sex(sample, ref_file["trained_cutoff"]) | ||
| if not ref_file["is_nipt"]: | ||
| if args.gender: | ||
| gender = args.gender | ||
| sample = sex_correct(sample, gender) | ||
| ref_gender = gender |
There was a problem hiding this comment.
--gender is declared as Sex (an Enum), but later gender is compared to string literals (e.g. gender == "M") and passed into sex_correct, which currently checks sex == "M". If the user provides --gender, gender becomes a Sex instance and these checks will fail (and gonosome normalization may select the wrong branch). Convert the enum to its .value when assigning/using it (or update sex_correct / comparisons to handle Sex).
There was a problem hiding this comment.
Fixed in 9a5cde1. Added .value when assigning args.gender to gender (gender = args.gender.value) in both the NIPT and non-NIPT branches, so gender is always a plain string ("M" or "F") consistent with what predict_sex returns and what the downstream comparisons and sex_correct expect.
| mode = "rb" if infile.suffix == ".bam" else "rc" | ||
| ref_filename = str(reference) if reference else None | ||
|
|
||
| if mode == "rc" and not ref_filename: | ||
| logging.error( | ||
| "Cram inputs need a reference fasta provided through the '--reference' flag." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Input format detection treats any non-.bam file as CRAM (mode = "rb" if ... else "rc"). This misclassifies unsupported extensions and produces a misleading “Cram inputs need a reference” error. Add an explicit suffix check for .bam/.cram and error out for anything else.
nvnieuwk
left a comment
nvnieuwk
left a comment
There was a problem hiding this comment.
I haven't checked the new files but I trust you didn't change anything there, the rest looks good!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/CenterForMedicalGeneticsGhent/WisecondorX/sessions/fc8b91aa-c1cc-4cd6-be6e-6bcf4762b913 Co-authored-by: matthdsm <11850640+matthdsm@users.noreply.github.com>
Agent-Logs-Url: https://github.com/CenterForMedicalGeneticsGhent/WisecondorX/sessions/dafe2c9e-9d88-41cd-bb97-c1ddb3bc942b Co-authored-by: matthdsm <11850640+matthdsm@users.noreply.github.com>
_generate_regions_bed- useelif/elsechain so X/Y assignments are not overwritten by theint(re.sub(...))conversion--genderSex enum comparison - extract.valuewhen assigningargs.genderso downstream string comparisons (== "M",== "F") work correctly