Pass high confidence BED file in the small variant analysis

[georgiakes]

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).

[georgiakes]

I would like to confirm whether regions_bed is used correctly with with the incoming changes.
Up until now, the regions_bed file was passed to -f. With the incoming changes this file will be used with -R.
This seems to agree with both the pipeline documentation and the happy documentation in the sense that that the are making clear that is works like bcftools -R . It this the functionality that we want to achieve?

[georgiakes]

However, in another place in the pipeline documentation we say that the regions_bed is for defining the high confidence regions. The happy documentation , though, states that the confidence bed should be passed form -f.
It is a little bit confusing. Should we maybe change the description of the regions_bed param?

[kubranarci]

I would like to confirm whether regions_bed is used correctly with with the incoming changes. Up until now, the regions_bed file was passed to -f. With the incoming changes this file will be used with -R. This seems to agree with both the pipeline documentation and the happy documentation in the sense that that the are making clear that is works like bcftools -R . It this the functionality that we want to achieve?

Hello, Passing regions_bed with -f was due to the errorious fix in the previous version. I confirm, we need to use -R in the pipeline to define regions file.

[kubranarci]

However, in another place in the pipeline documentation we say that the regions_bed is for defining the high confidence regions. The happy documentation , though, states that the confidence bed should be passed form -f. It is a little bit confusing. Should we maybe change the description of the regions_bed param?

I agree that is really confusing. Can you also please fix the documentation? regions_bed is not really defining confidence regions.. Better to add that note to -f (--false-positives bed)

[kubranarci]

@georgiakes You should also update the subworkflow tests involving happy.. :/

[georgiakes]

Hello! I have updated the documentation for some of the BED file parameters.

Since we updated the definition of regions_bed and agreed that falsepositive_bed will serve as our high-confidence evaluation region, we will also need to adjust our vcfeval implementation.

Instead of using regions_bed and targets_bed as the primary inputs for vcfeval, we should pass falsepositive_bed to the --evaluation-regions parameter , and then use --bed-regions with whichever subset (regions_bed or targets_bed) the user has provided as input.

Do you agree with this approach?

[kubranarci]

Hello! I have updated the documentation for some of the BED file parameters.
Since we updated the definition of regions_bed and agreed that falsepositive_bed will serve as our high-confidence evaluation region, we will also need to adjust our vcfeval implementation.
Instead of using regions_bed and targets_bed as the primary inputs for vcfeval, we should pass falsepositive_bed to the --evaluation-regions parameter , and then use --bed-regions with whichever subset (regions_bed or targets_bed) the user has provided as input.
Do you agree with this approach?

Hello, sorry I took me a while to understand what you mean :D

What you suggests makes me sense, but we should make this very clear to he user. Since, falsepositive_bed doesnt sound like high confidence. What would you think we also change the general input variable from falsepositive_bed to high_confidence_bed ? That will make it more clear?

[georgiakes]

I agree! The name as it right now doesn't help. high_confidence_bed is better.

[kubranarci]

Okay, lets do that, But we have to be carefull with all the processes using regions and targets files.. For example, cnv part also uses those non intuitively. But this will be a good change to make all more understandable.

[kubranarci]

It seems like benchmarking for vcfeval fails silently, that is why the snaphots are empty. I think it is due to the error in channel joining. I will take a deep look into it.

[kubranarci]

this snap seems empty

[kubranarci]

I am not sure join works here, I think it should be with combined (as meta is different join wont work)

[kubranarci]

there is also a typo here

[kubranarci]

this snap is also empty

[kubranarci]

this snapshots are not successful. seems like failed silently.

[kubranarci]

Thank you! that seems great now

[georgiakes]

Thanks! I have an additional thought though 😅
Is maybe the name high_confidence_bed misleading? In a way the user can use high confidence bed files for the other variant type analyses through the regions_bed param but I think this in not clear form the user perspective.
The other tools just do not have an -f or an --evaluation-regions equivalent and that is why they get the bed file from the more general regions_bed param.
Will something like small_variant_conf_bed make things a little more clear?
Sorry for bringing that up only now 😢

[kubranarci]

Thanks! I have an additional thought though 😅 Is maybe the name high_confidence_bed misleading? In a way the user can use high confidence bed files for the other variant type analyses through the regions_bed param but I think this in not clear form the user perspective. The other tools just do not have an -f or an --evaluation-regions equivalent and that is why they get the bed file from the more general regions_bed param. Will something like small_variant_conf_bed make things a little more clear? Sorry for bringing that up only now 😢

No problems :) I also have the similar concerns. I think we definitely need to enhance read me and input descriptions. For other tools, we are still using regions bed (which is evaluation-bed), as long as we make it clear that it is not high confidence bed it should be good. I also realised a bug in the pipeline (not related to your change actually), I was plaining to add those clarifications with that PR.

[kubranarci]

@georgiakes hey are you going to merge this PR? I am thinking about starting to add aardvark to the pipeline, dont want to create conflicts for you.

[georgiakes]

@kubranarci I think I will close is PR for now and create in another one with the different approach we discussed for the bed files. Feel fee to work on the aardvark.