Skip to content

Reorganize bed files for small variant analysis - #319

Merged
georgiakes merged 11 commits into
nf-core:devfrom
georgiakes:high-conf-for-small-analysis
Aug 11, 2026
Merged

Reorganize bed files for small variant analysis#319
georgiakes merged 11 commits into
nf-core:devfrom
georgiakes:high-conf-for-small-analysis

Conversation

@georgiakes

@georgiakes georgiakes commented Jul 31, 2026

Copy link
Copy Markdown
Member

I have updated the happy module to include the updated version.
Also, I have changed the bed handling to match the new usage documentation

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/variantbenchmarking branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions

github-actions Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 4.0.3.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the Synchronisation documentation.

@github-actions

github-actions Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit f775377

+| ✅ 302 tests passed       |+
#| ❔   3 tests were ignored |#
#| ❔   1 tests had warnings |#
!| ❗   2 tests had warnings |!
Details

❗ Test warnings:

  • schema_lint - Schema 'description' should be 'Variant Benchmarking pipeline for germline and somatic variant callers '
    Found: 'Variant Benchmarking pipeline for germline and somatic variant callers'
  • schema_lint - Input mimetype is missing or empty

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: docs/images/nf-core-variantbenchmarking_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-variantbenchmarking_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore

❔ Tests fixed:

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.3
  • Run at 2026-08-11 14:20:18

@georgiakes

georgiakes commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@kubranarci I think I like the idea of handling the bed files like this but I understand that is might make the code more complicated. I would like your input in that :)

@kubranarci

kubranarci commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@kubranarci I think I like the idea of handling the bed files like this but I understand that is might make the code more complicated. I would like your input in that :)

I kind of like it as it is quite simple and smooth solution. But, I also have some concerns of being apart from the benchmarking terms. I concern people might expect to find regular terms in the pipeline like false-positives regions_bed or targets_bed.

Otherwise, I think codewise it is all good.

@kubranarci

kubranarci commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

I realize now that we completely disregard using -R and -T together in happy (if together intersection is being used).

@kubranarci

kubranarci commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

I think my approach here would be following the standard names that benchmarking tools use:

(-R) regions_bedfile (happy,sompy,rtgtools,aardvark,and others)
(-T) targets_bedfile (happy,sompy)
(-f) confident_call_regions (happy,sompy) : -e for (rtgtools)

The main reason is as I think people might need to use -R and -f, or -T and -R, or -T and -F together. (I chatted with gemini and got some use cases.. ):

While -T and -R serve similar purposes (restricting the analysis footprint), they are optimized for different types of data. Combining -R with -f is the standard approach when you want to benchmark a very specific, sparse list of genomic locations while still respecting the truth set's confidence boundaries.. You should use hap.py -f confident_regions.bed -R sparse_targets.bed when you are evaluating a highly targeted assay (like a small clinical gene panel or an amplicon sequencing panel) against a gold-standard truth set like Genome in a Bottle.

Yes, you can use -f and -T together. In fact, it is the recommended approach when benchmarking targeted sequencing like whole exome sequencing (WES) or custom gene panels.

@georgiakes

georgiakes commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

I understand. The issue we had, though, with the confident_call_regions was that it might be confusing to the user. We will basically ask to put a confidence bed for happy and sompy to confident_call_regions and then the same file in -e for rtgtools but if someone runs any other tool, one has to put it on regions_bed.

@georgiakes

georgiakes commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

I understand the the different nomenclature between the tools doesn't help at all and using regions_bed for passing the -f argument on happy is confusing That is why I would also propose to rename our parameters avoid any confusion.

@georgiakes

georgiakes commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Also, I think I'm not against the idea of creating 2 targets bed file parameters. One called targets_dense and targets_sparse for example.

@kubranarci

kubranarci commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

I understand. The issue we had, though, with the confident_call_regions was that it might be confusing to the user. We will basically ask to put a confidence bed for happy and sompy to confident_call_regions and then the same file in -e for rtgtools but if someone runs any other tool, one has to put it on regions_bed.

It is confusing indeed :D I think we have to make it clear that confident_call_regions (happy,sompy) is -e (evaluation-region) for (rtgtools) and they dont exist in other tools.

We will be fixing using confident-calls as -R (that was the main aim of this PR) (in happy, sompy and rtgtools)

I think nomenclature for regions-bed is same for all tools, there is no confusion there.

@github-actions

github-actions Bot commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown

❌ nf-test failed with latest Nextflow version

Note

Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
Please check if the failure is expected with newer (edge-)releases of Nextflow or if it needs fixing.

  • ❌ docker | latest-everything | Shard 5/7

See the full run for details.

@georgiakes

georgiakes commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

Should we maybe set high_conf to true since that was the default up until now?

@kubranarci

kubranarci commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Should we maybe set high_conf to true since that was the default up until now?

it makes sense yes.

kubranarci
kubranarci reviewed Aug 11, 2026
View reviewed changes
Comment thread docs/usage.md

@kubranarci kubranarci Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the detailed explanation! looks amazing.

kubranarci
kubranarci reviewed Aug 11, 2026
View reviewed changes
Comment thread subworkflows/local/small_benchmark/main.nf
tagged_variants = channel.empty()
summary_reports = channel.empty()
tagged_variants = channel.empty()
falsepositive_bed = channel.empty()

@kubranarci kubranarci Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like falsepositive_bed should be [[],[]]

kubranarci
kubranarci reviewed Aug 11, 2026
View reviewed changes

@kubranarci kubranarci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic seems nice, but I am not sure if happy and sompy works properly. Did you perform small germline and somatic tests? then probably you would need to update the snapshots (might be producing the same results so I am not sure)

@georgiakes

georgiakes commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

Logic seems nice, but I am not sure if happy and sompy works properly. Did you perform small germline and somatic tests? then probably you would need to update the snapshots (might be producing the same results so I am not sure)

I'm running the tests right now.

kubranarci
kubranarci reviewed Aug 11, 2026
View reviewed changes
Comment thread subworkflows/local/small_benchmark/main.nf Outdated
Comment thread CHANGELOG.md
@kubranarci

kubranarci commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

you need to renew HAPPY_BENCHMARK subworkflow snapshot :)

kubranarci
kubranarci approved these changes Aug 11, 2026
View reviewed changes

@kubranarci kubranarci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to goo

@georgiakes
georgiakes merged commit 2400ba4 into nf-core:dev Aug 11, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kubranarci kubranarci kubranarci approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@georgiakes @kubranarci