Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor release: 1.1.0 with CNV benchmark subworkflow #164

Merged
merged 26 commits into from
Mar 7, 2025
Merged

Minor release: 1.1.0 with CNV benchmark subworkflow #164

merged 26 commits into from
Mar 7, 2025

Conversation

kubranarci
Copy link
Contributor

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

@kubranarci kubranarci requested a review from nvnieuwk March 7, 2025 10:47
Copy link

@luisas luisas left a comment

Choose a reason for hiding this comment

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

LGTM!

I left a couple of comments/questions - but they are not blockers, so I leave it approved :)

@@ -191,6 +191,29 @@ def get_happy_resuls(file_paths):

return merged_df

def get_intersect_resuls(file_paths):
Copy link

Choose a reason for hiding this comment

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

Suggested change
def get_intersect_resuls(file_paths):
def get_intersect_results(file_paths):

Copy link

Choose a reason for hiding this comment

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

Or is this meant to be called resuls ? I see you have it throughout the script so maybe I am missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a sytex mistake :D I realize it but leave it as it is now

truth_id = "SEQC2"
truth_vcf = "${params.test_data_base}/testdata/hg38/truth/somatic/cnv_benchmark_calls.vcf"
regions_bed = "${params.test_data_base}/testdata/hg38/truth/somatic/ngs_benchmark_cnv_gain_loss_loh.bed"
rename_chr = "${projectDir}/assets/rename_contigs/grch37_grch38.txt"
Copy link

Choose a reason for hiding this comment

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

Should this data be in nf-core/testdatasets? could it be useful for other modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in test-database, I am linking it through params.test_data_base

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this comment

Copy link

Choose a reason for hiding this comment

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

I meant the last row - it is in projectDir not test_data_base, so I was wondering whether there was a reason for that

Copy link

Choose a reason for hiding this comment

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

For some reason I highlighted everything :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_data_base = 'https://raw.githubusercontent.com/nf-core/test-datasets/variantbenchmarking'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already in test-datasets, that is what I meant :D

Copy link

Choose a reason for hiding this comment

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

this one too? "${projectDir}/assets/rename_contigs/grch37_grch38.txt"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is almost default for the pipeline itself, It is not test purposes.

@@ -0,0 +1,54 @@
process BEDTOOLS_INTERSECT {
Copy link

Choose a reason for hiding this comment

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

Is there no way to use https://nf-co.re/modules/bedtools_intersect ?

Or a patch to it? Or is it doing something different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The python script I have, is with more functionalities like checking the proper file formats, and summarizing statistics.

}else{
log.error "Please specify params.truth_id and params.truth_vcf to perform benchmarking analysis"
log.error "Please specify params.truth_id and params.truth_vcf or regions_bed to perform benchmarking analysis"
Copy link

Choose a reason for hiding this comment

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

Could this not be catched with nf-schema?

Copy link

Choose a reason for hiding this comment

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

Or is it to better error messages? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be possible, but I couldnt figure out how..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not being validated with nf-schema by the way. This is coming from nextflow_schema.json.

CHANGELOG.md Outdated
@@ -3,14 +3,37 @@
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## v1.0dev - [date]
## 1.1.0 - [06.03.2025]
Copy link

Choose a reason for hiding this comment

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

The date is of yesterday, but I guess it is not a big deal :) In case you decide to do a new PR t could be updated

@kubranarci kubranarci merged commit 2e34d50 into master Mar 7, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants