You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(cn): wire the copy-number intervals into the couple screen and the report, and close the review follow-ups - #288
Step 35 writes a copy-number interval table (<id>_cn_intervals.tsv), and the couple screen can read it to list the carrier deletion alleles and a male partner's AZF deletions. Nothing passed it in. Step 39 and the Nextflow couple screen left out the table and the CNVpytor calls that cross-check it, so those lines only showed up if you ran couple_report.py by hand. The Nextflow HTML report also never received the table, so summary.json had no interval rows in Nextflow even though the bash report reads them. Two beta-globin deletions were still missing, and step 35's docs did not list the table among its outputs.
Fix
Step 39 passes paralogs/<id>_cn_intervals.tsv (step 35) and cnvpytor/<id>_cnvs.vcf.gz (step 18) for each partner who has them.
COUPLE_CARRIER matches both files by name. main.nf hands them over from PARASCOPY and CNVPYTOR_VCF through two new emits, BAM_ANALYSIS.out.cn_intervals and SV.out.cnvpytor_vcf.
HTML_REPORT puts <id>_cn_intervals.tsv in paralogs/, where collect_summary.py looks for it, and main.nf joins it into the report inputs.
Two new interval rows use HbVar's GRCh38 spans: the 619 bp HBB deletion (HbVar 979, Orkin 1979, PMID 287080) and the 13.4 kb Sicilian (delta-beta)0 deletion (HbVar 1035, Henthorn 1990, PMID 2307466). I checked both spans by placing 50 bp of NG_000007.3 on each side of each breakpoint on NC_000011.10. Both agree with HbVar's hg38 positions. The couple screen lists them as two more alleles. docs/cn-loci.md says that the rows only count copies over their span: a Sicilian carrier reads as a deletion on all three HBB rows.
cn-intervals-2-hg002 now checks HG002's AZFc_DAZ result, not assessed with reason between_counts and an estimate between 3.3 and 3.9. It reads 3.63 on this head (fork run below).
The three beta-globin rows overlap: the Sicilian deletion covers both HBB rows, and the 619 bp and exon 3 rows share 487 bp. One deletion can read as present on two or three of them, so the couple report and docs/39-couple.md now say so, and that only a clinical test names the allele.
Step 18 counts in 1 kb bins and cannot see a deletion of about 600 bp. The couple report no longer prints "CNVpytor calls no deletion here" for an allele shorter than 2 kb (the two HBB rows), where that silence is no evidence.
docs/35-paralogs.md lists the interval table. docs/39-couple.md and docs/cn-loci.md no longer say the inputs are not passed.
Output changes, all additions: the interval table gets two new lines (HBB_619bp_del, HBD_HBB_Sicilian_del), and the couple report gets two new allele lines per partner. No column, file name or option was renamed or removed.
The optional fixture region over a breakpoint row (tests/test_copy_number.py) is not done. The catalog check already keeps every target piece off breakpoint rows.
Step 39 and COUPLE_CARRIER do not pass --cyp21a2 yet. feat(cn): call CYP21A2 deletions and gene conversions from short reads #280 added that option and merged after this PR was built; it is merged in here, but wiring it needs its own red controls and e2e checks, so it is left for a follow-up rather than added at the end of this one.
How it was checked
tests/test_copy_number.py and tests/test_couple_report.py: ALL PASS. Before the fix (the old bin/cn_loci.tsv and bin/couple_report.py), the two new copy-number checks (hand rows, HbVar spans) and the two new couple checks (five alleles, Sicilian and 619 bp present) fail.
scripts/ci/build-cn-intervals.py --check: "the 90 interval rows are what the pinned downloads give". Only the two new rows changed.
followups-cn-1-couple-report passes in shard 5, https://github.com/geiresx/OwnGenome/actions/runs/42134759461/job/127636984222 . Step 39 on a made-up couple lists the alleles and AZF lines with the CNVpytor note beside them. COUPLE_CARRIER writes the same report and table as step 39, byte for byte. HTML_REPORT puts the interval rows in summary.json. A stub run of the stub pedigree stages both files in COUPLE_CARRIER and the table in every HTML_REPORT task. Each wiring line has a red control in the case, and all of them log [PASS]: one partner's table removed, a module copy without the two input kinds, a module copy without the place line (exit 1), a pipeline copy without the two lines (nothing staged).
cn-intervals-2-hg002 passes in shard 3, https://github.com/geiresx/OwnGenome/actions/runs/42134759461/job/127636984198 . HG002 reads as male, AZFa 1.00 and AZFb 1.01 copies, AZFc_DAZ not assessed, between two whole counts, estimate 3.3 to 3.9 (3.63), 22q11.2 A-B 2.06. The same AZFc check, run on the female table, fails as it should (not applicable/no_y/out), and that control logs [PASS].
tests/test_couple_report.py has a new check that no missing-call note is printed for the 619 bp and 621 bp rows while the 13.4 kb Sicilian row keeps it. It failed on the code before the fix (RESULT: 1 FAILED, the note was on both HBB lines) and passes after.
The GJB6 pattern of the e2e case now escapes its parentheses. Against the exact report line, the old pattern gave grep exit 1 and the new one exit 0.
tests/test_copy_number.py has a new check that the summary keeps the interval rows with no SMN table: RESULT: 1 FAILED before the collect_summary.py change, ALL PASS after.
…een and the Nextflow report
Step 39 and COUPLE_CARRIER now hand couple_report.py each partner's
step 35 interval table and step 18 CNVpytor VCF when they exist, so the
carrier deletion alleles and AZF lines appear without a hand-run command.
HTML_REPORT places <id>_cn_intervals.tsv in paralogs/, where
collect_summary.py reads it, and main.nf joins it into the report inputs.
Bead: pgp-pfw6.36
HG002's DAZ depth gives an estimate of 3.63 in CI run 41767270253, so the
row is not assessed with reason between_counts. The case now checks that,
with the estimate held to 3.3 to 3.9, and checks that the same assertion
fails on the female table.
Bead: pgp-pfw6.36
…depth
Two interval rows at their HbVar GRCh38 spans (979 and 1035), checked by
placing the NG_000007.3 breakpoint flanks on NC_000011.10. The couple
screen lists both as carrier deletion alleles. The step 35 interval table
gains two lines; no existing row or column changes.
Bead: pgp-pfw6.36
Reviewing files that changed from the base of the PR and between ba87cb7 and 861dbb3.
Files ignored due to path filters: 2
bin/cn_loci.tsv is excluded by !**/*.tsv
scripts/ci/e2e-case-paths.tsv is excluded by !**/*.tsv
Files selected for processing: 18
bin/collect_summary.py
bin/couple_report.py
docs/35-paralogs.md
docs/39-couple.md
docs/cn-loci.md
main.nf
modules/local/couple/main.nf
modules/local/html_report/main.nf
scripts/39-couple.sh
scripts/ci/build-cn-intervals.py
tests/e2e/cn-intervals-1-catalog.sh
tests/e2e/cn-intervals-2-hg002.sh
tests/e2e/followups-cn-1-couple-report.sh
tests/test_copy_number.py
tests/test_couple_report.py
workflows/bam_analysis.nf
workflows/family.nf
workflows/sv.nf
Walkthrough
The change adds two HBB deletion alleles to the copy-number catalog and couple report. It also passes interval tables and CNVpytor VCFs through couple-screen workflows, stages interval tables for HTML reports, and adds related tests and documentation.
The couple report recognizes both deletions and notes that overlapping HBB rows can report one deletion more than once. Tests check present and absent results.
The workflows pass interval tables and CNVpytor VCFs to the couple screen and stage interval tables for HTML reports. End-to-end tests check the input paths, report output, and AZFc results.
GeiserX/OwnGenome#275: Adds CN interval and CNVpytor inputs and carrier findings to couple_report.py.
GeiserX/OwnGenome#276: Adds discovery and staging of per-sample copy-number tables in HTML reports.
GeiserX/OwnGenome#280: Adds another copy-number-derived carrier-screen input to couple_report.py.
Merge Risk: Minimal · previous review ba87c
No concrete merge-blocking defect is established. The new inputs follow the documented cross-check behavior; whether absent CNVpytor calls need additional interpretation for the 619 bp allele remains unresolved.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main change: wiring copy-number interval tables and CNVpytor calls into the couple screen and report. It also names the review follow-ups. The changeset matches this. The title is …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: wiring copy-number interval tables and CNVpytor calls into the couple screen and report. It also names the review follow-ups. The changeset matches this. The title is long (110 characters, 17 words), and the second clause is vague, but the primary change is clear.
Full details: Docstring Coverage
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 7 files. (9 skipped: 9 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
The reason will be displayed to describe this comment to others. Learn more.
Additional comments: 1
bin/couple_report.py-403-404: 1
403-404: Minor · Potential issue · Quick win
Unverified finding
We couldn't verify this from the code. It is shown for review, not as a verified issue.
Adding the 619 bp HBB allele makes the existing CNVpytor disagreement annotation apply to a sub-kilobase deletion. If step 18 cannot reliably call deletions that small, the report may present an expected lack of a CNVpytor call as contradictory evidence.
The couple report says “CNVpytor (step 18) calls no deletion here” beside a genuine depth-based finding, potentially misleading readers about the strength of the evidence.
This can occur when: Step 35 calls the HBB 619 bp deletion present and a CNVpytor VCF is supplied with no call over that span.
Reviewing files that changed from the base of the PR and between 4685227 and ba87cb7.
Files ignored due to path filters: 1
bin/cn_loci.tsv is excluded by !**/*.tsv
Files selected for processing: 16
bin/couple_report.py
docs/35-paralogs.md
docs/39-couple.md
docs/cn-loci.md
main.nf
modules/local/couple/main.nf
modules/local/html_report/main.nf
scripts/39-couple.sh
scripts/ci/build-cn-intervals.py
tests/e2e/cn-intervals-2-hg002.sh
tests/e2e/followups-cn-1-couple-report.sh
tests/test_copy_number.py
tests/test_couple_report.py
workflows/bam_analysis.nf
workflows/family.nf
workflows/sv.nf
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The unverified note on bin/couple_report.py 403-404 holds: step 18 calls on 1 kb bins, so it cannot see the 619 bp or 621 bp HBB deletions. Fixed in 0c490e0: the missing-call note is now printed only for an allele of 2 kb or more, with a unit check that was red before the fix.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
#288 added the case while a case could still go without a row; this PR
makes a missing row fail Guard, so the merge of main failed unit-tests.
69 s is its time in the passing fork run 42134759461; it reads only its
own folder, so it needs nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Problem
Step 35 writes a copy-number interval table (
<id>_cn_intervals.tsv), and the couple screen can read it to list the carrier deletion alleles and a male partner's AZF deletions. Nothing passed it in. Step 39 and the Nextflow couple screen left out the table and the CNVpytor calls that cross-check it, so those lines only showed up if you rancouple_report.pyby hand. The Nextflow HTML report also never received the table, sosummary.jsonhad no interval rows in Nextflow even though the bash report reads them. Two beta-globin deletions were still missing, and step 35's docs did not list the table among its outputs.Fix
paralogs/<id>_cn_intervals.tsv(step 35) andcnvpytor/<id>_cnvs.vcf.gz(step 18) for each partner who has them.COUPLE_CARRIERmatches both files by name.main.nfhands them over fromPARASCOPYandCNVPYTOR_VCFthrough two new emits,BAM_ANALYSIS.out.cn_intervalsandSV.out.cnvpytor_vcf.HTML_REPORTputs<id>_cn_intervals.tsvinparalogs/, wherecollect_summary.pylooks for it, andmain.nfjoins it into the report inputs.docs/cn-loci.mdsays that the rows only count copies over their span: a Sicilian carrier reads as a deletion on all three HBB rows.cn-intervals-2-hg002now checks HG002's AZFc_DAZ result,not assessedwith reasonbetween_countsand an estimate between 3.3 and 3.9. It reads 3.63 on this head (fork run below).docs/39-couple.mdnow say so, and that only a clinical test names the allele.docs/35-paralogs.mdlists the interval table.docs/39-couple.mdanddocs/cn-loci.mdno longer say the inputs are not passed.Output changes, all additions: the interval table gets two new lines (
HBB_619bp_del,HBD_HBB_Sicilian_del), and the couple report gets two new allele lines per partner. No column, file name or option was renamed or removed.Decisions:
tests/test_copy_number.py) is not done. The catalog check already keeps every target piece off breakpoint rows.collect_summary.pynow also keeps the interval rows when step 35 wrote no other table at all.scripts/ci/e2e-case-paths.tsvgets one row for the new case. ci(e2e): run only the e2e cases a pull request can affect #297 made a row there required for every case, so the case could not run without it.COUPLE_CARRIERdo not pass--cyp21a2yet. feat(cn): call CYP21A2 deletions and gene conversions from short reads #280 added that option and merged after this PR was built; it is merged in here, but wiring it needs its own red controls and e2e checks, so it is left for a follow-up rather than added at the end of this one.How it was checked
tests/test_copy_number.pyandtests/test_couple_report.py: ALL PASS. Before the fix (the oldbin/cn_loci.tsvandbin/couple_report.py), the two new copy-number checks (hand rows, HbVar spans) and the two new couple checks (five alleles, Sicilian and 619 bp present) fail.scripts/ci/build-cn-intervals.py --check: "the 90 interval rows are what the pinned downloads give". Only the two new rows changed.followups-cn-1-couple-reportpasses in shard 5, https://github.com/geiresx/OwnGenome/actions/runs/42134759461/job/127636984222 . Step 39 on a made-up couple lists the alleles and AZF lines with the CNVpytor note beside them.COUPLE_CARRIERwrites the same report and table as step 39, byte for byte.HTML_REPORTputs the interval rows insummary.json. A stub run of the stub pedigree stages both files inCOUPLE_CARRIERand the table in everyHTML_REPORTtask. Each wiring line has a red control in the case, and all of them log[PASS]: one partner's table removed, a module copy without the two input kinds, a module copy without the place line (exit 1), a pipeline copy without the two lines (nothing staged).cn-intervals-1-catalogpasses in shard 2 at 90 rows, https://github.com/geiresx/OwnGenome/actions/runs/42134759461/job/127636984447 , with its control (one moved target makes--checkexit 1). It went red on the old count of 88 in e2e shard 2 of run 41853902601.cn-intervals-2-hg002passes in shard 3, https://github.com/geiresx/OwnGenome/actions/runs/42134759461/job/127636984198 . HG002 reads as male, AZFa 1.00 and AZFb 1.01 copies,AZFc_DAZ not assessed, between two whole counts, estimate 3.3 to 3.9 (3.63), 22q11.2 A-B 2.06. The same AZFc check, run on the female table, fails as it should (not applicable/no_y/out), and that control logs[PASS].tests/test_couple_report.pyhas a new check that no missing-call note is printed for the 619 bp and 621 bp rows while the 13.4 kb Sicilian row keeps it. It failed on the code before the fix (RESULT: 1 FAILED, the note was on both HBB lines) and passes after.tests/test_copy_number.pyhas a new check that the summary keeps the interval rows with no SMN table:RESULT: 1 FAILEDbefore thecollect_summary.pychange,ALL PASSafter.scripts/ci/e2e-scope.sh checkpasses with the new case's row inscripts/ci/e2e-case-paths.tsv, and sayscase followups-cn-1-couple-report has no rowwithout it.Beads: pgp-pfw6.36 (pgp-19um was fixed by #276)