feat: preparation for somatic cnv #590

ericblanc20 · 2025-01-10T16:50:58Z

Adds several (fairly simple & simple-minded) steps required for proper CNV calling:

guess_sex: simple inference of sex for autosome & sex chromosome coverage
germline_snvs: simple identification of well-supported germline SNPs. The variant_calling step unfortunately cannot be used for this task, as it is designed for trios.
somatic_variants_for_cnv: creates input for cnv tools using B-allele fractions to improve/verify CNV calls based on coverage alone. The somatic_variant_calling step cannot be used, as the somatic variants from mutect2 differ greatly when germline variants are included or not.

The current code is OK, but can certainly be improved:

Pipes could be extended, but it needs more clever choices in germline_snvs/__init__.py
The problems with model serialization & validation should be understood & fixed: at the moment, automatic alias generation in bcftools models seems to work in isolation, when only the step model is considered. But validation fails during registration of sub-steps.
Better design of the snappy_wrapper is probably possible. Also, the derived BcftoolsWrapper is a first attempt at streamlining UNIX-like tools (such as bcftools, bedtools, bedops, samtools, rnaqc, ...). Its design should be critically reviewed, before similar wrappers are built.
The treatment of ignored_chroms should also be seen as a first attempt to be critically reviewed. The code in genome_windows is exercised in the ignored_chroms wrapper, called from the germline_snvs & somatic_variants_for_cnv snakefiles.

github-actions · 2025-01-10T16:52:54Z

Please format your Python code with ruff: make fmt
Please check your Python code with ruff: make check
Please format your Snakemake code with snakefmt: make snakefmt

You can trigger all lints locally by running make lint

coveralls · 2025-01-14T08:49:50Z

coverage: 80.761% (-5.0%) from 85.765%
when pulling 0fa6eed on 587-preparation-for-somatic-cnv
into f569530 on main.

…of Rscript, for more verbose logs

…to normal sample mapping

… BAQ, changed skip on flags), protect against None model values, changed regions option name

tedil

Phew, this is a lot! Thank you for all the work and effort that went into it:

Here are some remarks, questions & feedback, in no particular order:

make sure VCF_TAG_PATTERN and ANNOTATION_VCF_TAG_PATTERN cover the definitions of the VCF specification
try to use the model and its attributes where applicable instead of dict access (i.e. wf.config.property.… instead of config["step_config"][…])
use functions for getting resources and params/args, so we can more easily add dynamic resource estimation later on
I think "last" is an unfortunate name for an action, I'd prefer something akin to "finalize" or "gather_final_result" or …)
make use of dictify and listify consistently
should we produce a little illustration for the documentation on how the steps introduced here interact/intertwine/support one another?
avoid code duplication:
- I think _collapsed_arg_value and collapse_args appear multiple times with basically the same code; in this case, we may also find a cleaner way to do it, but it's fine for now!
- get_args is often the same 2 lines I think, is that not part of Base/AbstractStepXYZ already?
hardcoded extra_args seem weird to me, can they not be part of the default config instead?
instead of using the do_md5 argument, we could add an automatic check for this
TODO: check regular expressions for correctness
use named groups in regexes
for even more comments, see the respective line comments ;)

tedil · 2025-01-14T10:13:47Z

snappy_pipeline/models/bcftools.py

+    SNP_INS_DEL = "snp-ins-del"
+    """Used in merge"""
+    ID = "id"
+    """Used in merge"""
+    STAR = "*"
+    """Used in merge"""
+    STAR_STAR = "**"
+    """Used in merge"""


Split into two (or more) enums? (And use the union of them where applicable?)

I have added command-dependent variant type classes. I am not sure it is the best solution, but I think at least it is clear.
As the model is not used anywhere (at the moment), I think I'll keep it as it is, unless you have reservations.

snappy_pipeline/workflows/common/samplesheet.py

tedil · 2025-01-14T10:18:47Z

snappy_pipeline/workflows/common/samplesheet.py

+    assert not any(table.duplicated()), "Duplicated entries in sample sheets"
+    assert not any(table["ngs_library"].duplicated()), "Duplicated NGS libraries"
+
+    # table.set_index("ngs_library", drop=False, inplace=True)


Now I'm wondering if there is any column which is useful to set as (default) index…

Yes, you are absolutely right. I was caught by my ignorance of pandas. But actually, it makes a lot of sense to index the table by the ngs library name.

tedil · 2025-01-14T10:25:24Z

snappy_pipeline/workflows/germline_snvs/Snakefile

+
+rule germline_snvs_bcftools_ignore_chroms:
+    input:
+        reference=config["static_data_config"]["reference"]["path"],


I think we could also use wf.w_config.static_data_config.reference.path instead.

Much better indeed. Done

tedil · 2025-01-14T10:26:54Z

snappy_pipeline/workflows/germline_snvs/Snakefile

+        **{
+            "args": {
+                "ignore_chroms": set(
+                    config["step_config"]["germline_snvs"]["ignore_chroms"]


Similarly, wf.w_config.step_config["germline_snvs"].ignore_chroms etc etc.

tedil · 2025-01-21T14:09:14Z

snappy_wrappers/wrappers/bcftools/merge_germline_and_somatic/wrapper.py

+args = getattr(snakemake.params, "args", {})
+
+cmd = r"""
+bcftools iseq \


Suggested change

bcftools iseq \

bcftools isec \

tedil · 2025-01-21T14:10:57Z

snappy_wrappers/wrappers/bcftools/merge_germline_and_somatic/wrapper.py

+
+awk \
+    -F '\t' \
+    '($5 != "11") && (length($3) == 1) && (length($3) == length($4)) {{printf "%s\t%d\t%d\n", $1, $2-1, $2}}' \


I would love having some documentation on what this is supposed to filter / produce etc

tedil · 2025-01-21T14:12:35Z

snappy_wrappers/wrappers/ignore_chroms/environment.yaml

+  - bioconda
+  - nodefaults
+dependencies:
+  - htslib


We should at least put the current version as a lower limit (+ an upper limit)

tedil · 2025-01-21T14:13:48Z

snappy_wrappers/wrappers/ignore_chroms/wrapper.py

A little docstring explaining what this wrapper does / what it is used for would be nice!

tedil · 2025-01-21T14:16:14Z

snappy_wrappers/wrappers/samtools/guess_sex/environment.yaml

Since you're using grep and awk etc, make sure to also add e.g. coreutils or just awk and grep as their own tools.

I have added coreutils dependency, and set the version to the same as snappy's. I did the same for samtools (same version as snappy).
Should we try to enforce version match for those basic tools that also are in snappy (bcftools, samtools, htslib, pysam, coreutils)? At least for simple wrappers, that don't require anything else?

Co-authored-by: Till Hartmann <[email protected]>

…ocation

…variants_for_cnv steps

…aside (because it's not quite complete)

… workflow config attributes & removed unnecessary resource allocation

ericblanc20 linked an issue Jan 10, 2025 that may be closed by this pull request

Preparation for somatic CNV #587

Open

ericblanc20 requested a review from tedil January 10, 2025 16:51

ericblanc20 changed the title ~~587 preparation for somatic cnv~~ feat: preparation for somatic cnv Jan 10, 2025

ericblanc20 added 17 commits January 16, 2025 13:08

feat: sample sheet to pandas table

9d7abc8

feat: added helper step guess_sex

7879406

feat: Support for ignore_chroms

0d78653

feat: conditional execution of md5 (for pipes) & revert to R instead …

f18be4a

…of Rscript, for more verbose logs

feat: Added boilerplate support for bcftools wrappers, and for tumor …

8cd477c

…to normal sample mapping

feat: Complete bcftools arguments, except for files & threads

47635e3

feat: Added germline_snvs step

787e6c0

fix: tool as argument rather than instance variable

ff74de4

fix: ngs_library not index anymore

a951f5b

refactor: better naming of basic bcftools command wrappers

fb1f76b

refactor: modifed default models (added ploidy, header lines, removed…

5a5cc52

… BAQ, changed skip on flags), protect against None model values, changed regions option name

feat: Added the step preparing vcf files for CNV tools

f27af0e

feat: enable germline_snvs & somatic_variants_for_cnv steps

d3a3cf5

style: Make ruff 0.9 happy: join strings

e951bd4

style: Make ruff 0.9 happy: reformat assertions

f825ddb

style: Make ruff 0.9 happy: changes in line lengths

74e0044

style: make snakefmt 0.10.2 happy: increase indentation

9660c7c

tedil force-pushed the 587-preparation-for-somatic-cnv branch from 5d0043e to 9660c7c Compare January 16, 2025 12:11

tedil requested changes Jan 21, 2025

View reviewed changes

ericblanc20 and others added 6 commits January 21, 2025 15:47

Update snappy_pipeline/workflows/common/samplesheet.py

9baa306

Co-authored-by: Till Hartmann <[email protected]>

Update snappy_pipeline/workflows/somatic_variants_for_cnv/__init__.py

4b0b37f

Co-authored-by: Till Hartmann <[email protected]>

refactor: finds snappy installation root directory from any wrapper l…

1be71c3

…ocation

docs: Adds some background on how to use germline_variants & somatic_…

49b7270

…variants_for_cnv steps

refactor: create a generic bcftools model, and push the complete one …

866b8a7

…aside (because it's not quite complete)

build: pin versions

1cd1624

ericblanc20 added 5 commits January 22, 2025 19:28

refactor: sample sheets indexed by ngs library name

2cb177b

style: Nicer parameter definition, renamed last to remove_unseen, use…

36ae59c

… workflow config attributes & removed unnecessary resource allocation

refactor: renamed 'last' action to 'remove_unseen'

c0d6722

refactor: use generic bcftools model

9011827

refactor: remove merge confict (in docstring)

0fa6eed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preparation for somatic cnv #590

feat: preparation for somatic cnv #590

ericblanc20 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

coveralls commented Jan 14, 2025 •

edited

Loading

tedil left a comment

tedil Jan 14, 2025

ericblanc20 Jan 21, 2025

tedil Jan 14, 2025

ericblanc20 Jan 21, 2025

tedil Jan 14, 2025

ericblanc20 Jan 21, 2025

tedil Jan 14, 2025

tedil Jan 21, 2025

tedil Jan 21, 2025

tedil Jan 21, 2025

tedil Jan 21, 2025

tedil Jan 21, 2025

ericblanc20 Jan 22, 2025

feat: preparation for somatic cnv #590

Are you sure you want to change the base?

feat: preparation for somatic cnv #590

Conversation

ericblanc20 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

coveralls commented Jan 14, 2025 • edited Loading

tedil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 14, 2025 •

edited

Loading