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.

Sign up for GitHub

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

Jump to bottom

Refactor parameters validation #69

Merged
merged 40 commits into from
Apr 17, 2024
Merged

Refactor parameters validation #69

merged 40 commits into from
Apr 17, 2024

Conversation

fellen31
Copy link
Collaborator

@fellen31 fellen31 commented Apr 5, 2024

This is more of a nice to have than need to have. E.g. give an error before starting the pipeline if a --skip_workflow is missing, rather than silently not running the workflow. Or give an error before starting the pipeline if a conditionally required file is not set, rather than giving a module error in the run.

Maybe this adds unneccesary complexity, but it also removes a lot of if else statements from workflows/skierfe.nf.

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
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • 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).

@fellen31
FastQC updated to allow more than 512MB memory per thread
6e50215
@fellen31
Allow bcftools merge to run on a single file
f42334e
@fellen31
Memory requirements shouldn't be hardcoded
dfd2c89
@fellen31
Add BED-file usage in modkit/pileup
03b72f4
@fellen31
Move dipcall alignment argument to conf, needed for low-memory test p…
722ea5d 
…rofile
@fellen31
Allow reference to be gzipped
fdf5521
@fellen31
forgot to add module
b8f5b86
@fellen31
Merge dev into prepare-pipeline-for-test-profile
e928c57
@fellen31
Lower memory requirements for assembly
4b1da4b
@fellen31
Add docker runoptions
58a8f67
@fellen31
add comment
74483a4
@fellen31
Update changelog
21d09dc
@fellen31
Initial template commit
8305665
@fellen31
merge template
5cb61a3
@fellen31
merge
4409265
@fellen31
patch bcftools/merge
a528dd7
@fellen31
working merge
4ea6471
@fellen31
update modules.json
175a98c
@fellen31
remove acidentally (?) commited orig-files
a775cb1
@fellen31
refactor parameters validations
bcaf240
@fellen31
merge
95265ac
@fellen31
wip
844f50f
@fellen31
wip
c4c9256
@fellen31
fix merge mistakes
b87b468
@fellen31
fix more merge mistakes
06d8d9f
@fellen31
typo
f8cd385
@fellen31
small fixes
9b85dc5
@fellen31
remove file
44f2827
@fellen31
regenerate fastq patch
b7ba23c
@fellen31
Update CHANGELOG
3042d23
@fellen31 fellen31 marked this pull request as ready for review April 5, 2024 10:55
@fellen31 fellen31 requested a review from jemten as a code owner April 5, 2024 10:55
@fellen31 fellen31 requested a review from peterpru April 5, 2024 10:55
peterpru
peterpru approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@peterpru peterpru left a comment

Choose a reason for hiding this comment

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

It looks good to me. From my stand point you have appropriate data structures and variable assignments, however I recommend a review from @ramprasadn or @jemten for more in-depth look on the code.

CHANGELOG.md Outdated Show resolved Hide resolved
workflows/skierfe.nf Outdated Show resolved Hide resolved
fellen31 and others added 2 commits April 8, 2024 17:02
@fellen31 @peterpru
Update CHANGELOG.md
1a14fd9 
Co-authored-by: Peter Pruisscher <[email protected]>
@fellen31 @peterpru
Update workflows/skierfe.nf
f1ccc65 
Co-authored-by: Peter Pruisscher <[email protected]>
@jemten jemten requested a review from ramprasadn April 9, 2024 13:50
ramprasadn
ramprasadn reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@ramprasadn ramprasadn left a comment

Choose a reason for hiding this comment

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

Nice work! Just a couple of minor comments.

subworkflows/local/utils_nfcore_skierfe_pipeline/main.nf Outdated Show resolved Hide resolved
subworkflows/local/utils_nfcore_skierfe_pipeline/main.nf Outdated Show resolved Hide resolved
jemten
jemten reviewed Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@jemten jemten left a comment

Choose a reason for hiding this comment

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

Generally, it's a nice addition to fail early when non compatible options have been given. However I think the maps are a little hard to follow.

subworkflows/local/utils_nfcore_skierfe_pipeline/main.nf Outdated Show resolved Hide resolved
@fellen31
Copy link
Collaborator Author

Updated the PR @jemten.

Split the dependencies map into three separate, and tried to remove much of the "active workflow" vs "--skip" confusion.

jemten
jemten reviewed Apr 16, 2024
View reviewed changes
Copy link
Collaborator

@jemten jemten left a comment

Choose a reason for hiding this comment

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

Looks good! So much easier for me to follow 😆

subworkflows/local/utils_nfcore_skierfe_pipeline/main.nf Outdated
nextflow_cli_args // array: List of positional nextflow CLI args
outdir // string: The output directory where the results will be saved
input // string: Path to input samplesheet
take:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the indentation has disappeared for this code block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@fellen31 fellen31 merged commit 5313950 into genomic-medicine-sweden:dev Apr 17, 2024
6 checks passed
@fellen31 fellen31 deleted the refactor-parameters-validation branch April 17, 2024 07:24
fellen31 added a commit that referenced this pull request May 8, 2024
@fellen31 @jemten
@ramsainanduri @peterpru @pbiology
Release 0.1.0 (#90)
eda623d 
* Add methylation calls to bedmethyl

* Output whatshap stats, and add pipeline presets

* Fix mosdepth not running for all samples

* Fix methylation sample mix

* fix dipcall not working

* Add fastq and bam stats

* fix dipcall not working

* Rework modules and subworkflows to match new-found knowledge about single element channels

* Added TRGT for pacbio-data

* Fix repeat versions

* Fix samtools index publishdirs

* Add back --secondary=no to minimap2 align

* Fix cramino phased and unphased

* Update README.md

* Fix dipcall sample name in vcf

* Fix trgt container adress

* try again to fix trgt container adress

* remove TRVZ because it will create too many processes

* completely remove TRVZ

* update modules conf, missing output files, index bams and fix containers

* Fix minimap2 index always using default params

* minimap index needs to be built separate for dipcall as well

* tweak process requirements

* Improve whatshap phasing

* Add hiphase

* add missing bcftools modules

* Add samplesheet, extra_gcvf and extra_snfs validation

* update hifiasm

* completely remove PED and rely on samplesheet

* fix methylation bug

* update modkit, removes now unneccesary creation of haplotype bams

* Formatting, remove unneccessary inputs

* fix leftover flag in pileup

* Add skip for phasing and remove more PED references

* Simplify sniffles, and put back dipcall par

* Update README.md

* Add skip for QC

* Update hiphase output files

* Add hificnv to workflow

* nf-download not working..

* add config

* Add missing file column to snfs schema

* Add hificnv to workflow

* nf-download not working..

* add config

* Update README.md

Remove reference to ped-file

* Update README.md

* Update align.nf

If using BAM-files as input for pbmm2, then don't output those.

* Replaces pbmm2 with minimap2, using the nf-core modules

* Fix typo

* Add missing diff..linting fails because of the container URL, not sure how to fix?

* linting working for all but samtools/faidx

* Update README.md

* Added DeepTrio support (incl. GPU for both DV and DT), trios extractedfrom PED-file

* Update README.md

* Small clean up.

* Add deeptrio, GPU support and fastqc (should get better at keeping features separate)

* fix minimap2 singularity version

* Added dipcall

* Add dipcall as process

* Might have messed up git stash

* Rewrite fix_trio process in Groovy, add hifiasm-trio and dipcall

* Merge updated TEMPLATE

* Update modules after TEMPLATE upgrade

* More modules fixes to bring it closer to nf-core

* Prettier

* Replace fix_trio process for deeptrio input

* remove nf-core images and add pipeline summary

* Update README.md

* fix color

* Allow to skip either assembly or read_map calling

* Update docs

* Remove references to non-existing slack

* Add tandem-regions and reference back to sniffles + skip snv-calling

* reorganise publishdirs

* move yak-versions into trio workflow

* Fix non-trio hifiasm and add more rememberable skips

* Fix dipcall module?

* Update genome_assembly.nf

* See if this way works

* See if this way works-2

* See if this way works-3

* nf-core download would not pull docker image when the container is specified inside single quotes

* Update dipcall to use PED-file in non-trio mode

* Update base config to better represent needs

* Change hifiasm to nf-core module, but make it output lowQ beds as well

* add mosdepth

* fix dipcall bug

* Revert "fix dipcall bug"

This reverts commit 36b9768.

* Revert "Revert "fix dipcall bug"" - sorry git log

This reverts commit 713272e.

* Hope to have fixed all dipcall bugs - but need more testing

* Change local GLNexus to nf-core module

* Make tandem_repeats optional, since it might not be that great?

* Update README.md

* Add methylation calls to bedmethyl

* Output whatshap stats, and add pipeline presets

* Fix mosdepth not running for all samples

* Fix methylation sample mix

* fix dipcall not working

* Add fastq and bam stats

* fix dipcall not working

* Rework modules and subworkflows to match new-found knowledge about single element channels

* Added TRGT for pacbio-data

* Fix repeat versions

* Fix samtools index publishdirs

* Add back --secondary=no to minimap2 align

* Fix cramino phased and unphased

* Fix dipcall sample name in vcf

* Update README.md

* Fix trgt container adress

* try again to fix trgt container adress

* remove TRVZ because it will create too many processes

* completely remove TRVZ

* update modules conf, missing output files, index bams and fix containers

* Fix minimap2 index always using default params

* minimap index needs to be built separate for dipcall as well

* tweak process requirements

* Improve whatshap phasing

* Add hiphase

* add missing bcftools modules

* Add samplesheet, extra_gcvf and extra_snfs validation

* update hifiasm

* completely remove PED and rely on samplesheet

* fix methylation bug

* update modkit, removes now unneccesary creation of haplotype bams

* Formatting, remove unneccessary inputs

* fix leftover flag in pileup

* Add skip for phasing and remove more PED references

* Simplify sniffles, and put back dipcall par

* Update README.md

* Add skip for QC

* Update hiphase output files

* Add missing file column to snfs schema

* Update README.md

Remove reference to ped-file

* Annotate variants with one database

* Change to list of dbs

* Fix anno overwriting input with output

* Fix anno output file

* Add VEP annotation

* Update README and schema etc.

* Add 'hack' to be able to run ONT-files in TRGT

* Update README

* Update README.md

* Update README.md

* Split alignment for faster processing

* Call variants per regions in bed/fai

* Extracting regions from extra gVCFs takes too long

* Singularity image does not contain jemalloc

* Grab correct one

* glnexus will load full files into db even with bed

* split DV input in 13 instead of n regions in BED

* forgot wf

* Clean up, remove PMD/DT as snv-callers, and ONT_R9 as preset

* Remove index split alignments & update README

* Update README

* Update README.md

* Update README.md

* Add grouptuple size so downstream wf's don't have to wait for all samples to complete

* pipeline would freeze depending on number of regions specified

* better outputdirs for aligned reads

* Update docs with pipeline parameters

* Update README

* Update README.md

* Remove nf-core reference

* Split module configs

* Fix most process outdirs

* minimap2 soft-clips supplementary reads by default

* update hificnv config

* Add expected CN-files (for XX/XY), MAF-vcf and option to use an exclude regions file. Also update HiFiCNV version to v.0.1.7

* Use BED file for mosdepth

* Fix mosdepth running without BED

* Update samtools_cat_sort_index.nf

Fix args bug

* Fix sniffles args bug

* Update short_variant_calling.config

Merged SNV-vcfs output files were misssing

* Fix actually skipping CNV workflow is specifying so

* Update qc_aligned_reads.nf

Fix mosdepth bed input

* Update qc_aligned_reads.nf

* Update split_bed_chunks.py

Sometimes, the BED-output was with decimals. Difference between system?

* Update split_bed_chunks.py

* Update split_bed_chunks.py

Check that BED-file does not contain any spaces instead.

* Update README.md

* adding codeowners

* Add test profile and data (#33)

Add small test profile and data, hg38 asset files and code formatting.

* Update pipeline to run with a small test dataset (#35)

Reduce memory requirements, allow gzipped reference, single sample run and add BED file in modkit.

* Template merge 2.13.1 (#38)

Merge template version 2.13.1

* Fix bcftools merge (#43)

Fix bcftools merge
---------

Co-authored-by: Anders Jemt <[email protected]>

* Add Revio BAM test data and a multisample-BAM samplesheet (#50)

Add BAM test data and a multisample-BAM samplesheet

* Add support for uBAM and multisample test (#51)

Add ubam support and multisample test

* Update usage docs (#53)

update usage docs

---------

Co-authored-by: Anders Jemt <[email protected]>

* use biocontainer for glnexus

* change to pacbio registry

* run pre-commit

* Update CHANGELOG

* Fixed mosdepth input channel bug when bedfile is not provided

* Update module configs (#65)

Update config output directories and add minimal documentation

* Fix checking whether input files exist (#67)

* Add back files exist check, and fromSamplesheet

* Add CNV calling workflow to test profile  (#68)

* Don't skip cnv calling workflow

* Updated CHANGELOG.md

* Add SNV annotation test data (#76)

* Fix path in snp_dbs.csv (#77)

* Refactor parameters validation (#69)

* Refactor parateters validation

---------

Co-authored-by: Peter Pruisscher <[email protected]>

* Add SNV annotation to test profile (#75)

* fix version reporting (#84)

* Run a single phasing software instead of three in parallel (#83)

* Remove leftover print (#88)

* Release prep v.0.1.0 (#89)

* bump versions
* clear changelog

* Name change: Skierfe -> Nallo   (#39)

Co-authored-by: Anders Jemt <[email protected]>

* Change testdata location (#91)

* move test data
* add dummy samplesheet

* Update sniffles module (#92)

Co-authored-by: Anders Jemt <[email protected]>

* update deepvariant module (#93)

* Add stubs (#94)

* update download pipeline (#95)

* Update README.md

* Update README.md

* Update docs/usage.md

* Update docs/usage.md

* Update docs/usage.md

* Update docs/usage.md

* Update docs/usage.md

* Delete annotations directory (#97)

* Delete assets/test_data directory (#99)

* Update download_pipeline.yml (#96)

* Update snv_annotation.config (#98)

Full size test fails because only `BCFTOOLS_NORM_SINGLESAMPLE` and not `BCFTOOLS_NORM` was actually decomposing variants.

* Update conf/modules/structural_variant_calling.config

Co-authored-by: Anders Jemt <[email protected]>

* Apply suggestions from code review and full test issues (#107)

* Add review suggestions + full test changes

* Update CODEOWNERS (#112)

* Update CODEOWNERS

Change the code-owners to the GitHub team. This way we can more easily change the team and not having to update the CODEOWNERS

* Fixed org

* Update whatshap stats version to avoid ZeroDivisionError (#133)

* Update whatshap stats version to avoid ZeroDivisionError

* Update release date

* Update bcftools merge (#134)

* Bump date in changelog

* Since sex is now numberic, update branching criteria

* Was fixed before, but was overwritten

---------

Co-authored-by: Anders Jemt <[email protected]>
Co-authored-by: Ram Nanduri <[email protected]>
Co-authored-by: Peter Pruisscher <[email protected]>
Co-authored-by: Anders Lind <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jemten jemten jemten left review comments

@ramprasadn ramprasadn ramprasadn left review comments

@peterpru peterpru peterpru 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.
4 participants
@fellen31 @jemten @ramprasadn @peterpru