Rename input file parameters #488

fellen31 · 2024-11-06T08:50:37Z

Closes #324.

Tries to make the file inputs to the pipeline more descriptive and uniform, e.g. prefacing the parameter with the tool name it belongs to.

PR checklist

Schmytzi

Great work!
However, I found some spots where you missed renaming bed to target_bed and some other minor things and inconsistencies.
I've got some more thoughts:

If I understood correctly, target_bed is only used for SNV calling. In that case, it should probably be called snv_calling_target_bed (or ..._regions, see one of the line comments)
Maybe it makes more sense to change skip_methylation_analysis to skip_methylaytion_pileup as that is all that subworkflow does. Or do we expect it to gain more functionality later on.

CHANGELOG.md

Schmytzi · 2024-11-07T13:45:15Z

CHANGELOG.md

+| `bed`                            | `target_bed`                      |
+| `hificnv_xy`                     | `hificnv_expected_xy_cn`          |
+| `hificnv_xx`                     | `hificnv_expected_xx_cn`          |
+| `hificnv_exclude`                | `hificnv_excluded_regions`        |


Maybe this should be hificnv_exluded_bed instead? The other bed param is called target_bed and not target_regions

OTOH, another parameter is called par_regions. So maybe target_bed should be target_regions instead

Third thought: "PAR regions" is redundant, as the "R" in "PAR" already means "region". So maybe it's better to use _bed for all of them.

I think both deepvariant and dipcall refer to them as PAR regions, so I'd prefer keeping it that way. Even though technically it's redundant (Swedish CD-skiva is another example).

But I think we could have target_regions instead of target_bed.

CHANGELOG.md

Schmytzi · 2024-11-07T13:51:08Z

CHANGELOG.md

+| `score_config_snv`               | `genmod_score_config_snvs`        |
+| `score_config_sv`                | `genmod_score_config_svs`         |
+| `parallel_alignments`            | `alignement_processes`            |
+| `svdb_dbs`                       | svdb_sv_databases``               |



Maybe variant_consequences_snv should be variant_consequences_snvs. The corresponding param is variant_consequences_svs and the similar param for genmod is genmod_score_config_snvs.

Sound good!

Schmytzi · 2024-11-07T13:52:19Z

CHANGELOG.md

@@ -119,6 +119,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 | `--validationSkipDuplicateCheck` |                                   |
 | `--validationS3PathCheck`        |                                   |
 | `--monochromeLogs`               | `--monochrome_logs`               |
+| `skip_short_variant_calling`     | `skip_snv_calling`                |


All parameters below are missing the --

Schmytzi · 2024-11-07T14:01:34Z

docs/parameters.md

-| `variant_catalog` | A variant catalog json-file for stranger | `string` |  |  |  |
+| `echtvar_snv_databases` | A csv file with echtvar databases to annotate SNVs with | `string` |  |  |  |
+| `svdb_sv_databases` | Databases used for structural variant annotation in vcf format. <details><summary>Help</summary><small>Path to comma-separated file containing information about the databases used for structural variant annotation.</small></details>| `string` |  |  |  |
+| `stranger_repeat_catalog` | A variant catalog json-file for stranger | `string` |  |  |  |
 | `variant_consequences_snv` | File containing list of SO terms listed in the order of severity from most severe to lease severe for annotating genomic SNVs. For more information check https://ensembl.org/info/genome/variation/prediction/predicted_data.html | `string` |  |  |  |
 | `variant_consequences_svs` | File containing list of SO terms listed in the order of severity from most severe to lease severe for annotating genomic SVs. For more information check https://ensembl.org/info/genome/variation/prediction/predicted_data.html | `string` |  |  |  |
 | `vep_cache` | A path to the VEP cache location | `string` |  |  |  |
 | `bed` | A BED file with regions of interest, used to limit short variant calling. | `string` |  |  |  |


Schmytzi · 2024-11-07T14:03:28Z

docs/usage.md

+| Parameter                   | Description                                                                                                                                                                                                                                                 |
+| --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `genmod_score_config_svs`   |  Used by GENMOD when ranking variants. Sample file [here](https://github.com/nf-core/test-datasets/blob/raredisease/reference/rank_model_snv.ini).                                                                                                          |
+| `genmod_reduced_penetrance` | A list of loci that show [reduced penetrance](https://medlineplus.gov/genetics/understanding/inheritance/penetranceexpressivity/) in people. Sample file [here](https://github.com/nf-core/test-datasets/blob/raredisease/reference/reduced_penetrance.tsv) |

 `--skip_rank_variants`.

 ## Other highlighted parameters

 - Limit SNV calling to regions in BED file (`--bed`).


Suggested change

- Limit SNV calling to regions in BED file (`--bed`).

- Limit SNV calling to regions in BED file (`--target_bed`).

Schmytzi · 2024-11-07T15:05:43Z

One more thing: The workflow parameter for skipping short-variant calling is skip_short_variant_calling but in all other params they are referred to as SNVs, even if they also relate to indels e.g., skip_snv_annotation.

Co-authored-by: Daniel Schmitz <[email protected]>

fellen31 · 2024-11-11T12:28:37Z

One more thing: The workflow parameter for skipping short-variant calling is skip_short_variant_calling but in all other params they are referred to as SNVs, even if they also relate to indels e.g., skip_snv_annotation.

Yeah. Here I wanted to rename skip_short_variant_calling to skip_snv_calling, to align with that they are called snvs in other parameters. I don't think it's ideal to call them snvs even though they contain indels as well.., but I don't know if it's more complicated to have "small variants", "snvs and indels", "snvs and small indels" in the parameters/file names? Any suggestion?

fellen31 · 2024-11-11T12:36:32Z

Great work! However, I found some spots where you missed renaming bed to target_bed and some other minor things and inconsistencies. I've got some more thoughts:

If I understood correctly, target_bed is only used for SNV calling. In that case, it should probably be called snv_calling_target_bed (or ..._regions, see one of the line comments)

Right now target_bed is used to limit SNV calling and methylation pileups, making the SNV-calling and methylation pileups quicker for targeted analysis. It's also passed to mosdepth to calculate depth per region. Ideally this bed would limit SV calling to the target regions, but now it's only filtering the results.

In Stockholm we will need to be able to output both variants from the whole genome, and filtered variants at the same time. I believe this would not be the usual use case for others, so perhaps we could have for example both a target_bed parameter and an snv_calling_target_bed. The snv_calling_target_bed is then by default set to target_bed, so a normal user get's expected results, but can also be overridden. Need to think about it some more.

Maybe it makes more sense to change skip_methylation_analysis to skip_methylaytion_pileup as that is all that subworkflow does. Or do we expect it to gain more functionality later on.

Yes, although it would be more correct currently, I hope we can at least add some visualisation track outputs to the workflow fairly soon. Even so, skip_methylation_analysis feels a bit broad maybe, because there's not much of an analysis going on.

Schmytzi · 2024-11-13T13:22:44Z

Regarding your first point: I think calling everything "SNV" is preferable. Even if it isn't technically correct I think it's clear what it means and I like conciseness.
Ad 2: That sounds like a good alternative.
Ad 3: Maybe skip_methylation_summary?

fellen31 · 2024-11-14T10:07:52Z

Regarding your first point: I think calling everything "SNV" is preferable. Even if it isn't technically correct I think it's clear what it means and I like conciseness.

Let's go with snv then.

Ad 2: That sounds like a good alternative.

I made a separate issue for finer-grained control over regions (#503). Renaming --bed to --target_regions in this PR.

Ad 3: Maybe skip_methylation_summary?

Let's go with your suggestion of --skip_methylation_pileups, because that is what is does. If it changes, then we can change the parameter if needed.

Schmytzi

Would you mind fixing the alignment where I marked it? 😬

nextflow.config

Co-authored-by: Daniel Schmitz <[email protected]>

fellen31 · 2024-11-15T07:44:57Z

Would you mind fixing the alignment where I marked it? 😬

Thanks!

fellen31 · 2024-11-18T08:19:28Z

Fixed!

jemten

Looks good to me! Anything else you like to add @Schmytzi?

Schmytzi

Looks great!

fellen31 added 2 commits November 6, 2024 09:34

rename parameters

8c42d97

formatting

a98b217

fellen31 force-pushed the rename-parameters branch 2 times, most recently from d559c7b to 75ecec7 Compare November 6, 2024 09:45

fix files

86f6604

fellen31 force-pushed the rename-parameters branch from 75ecec7 to 86f6604 Compare November 6, 2024 09:47

fellen31 marked this pull request as ready for review November 6, 2024 09:59

fellen31 requested a review from a team as a code owner November 6, 2024 09:59

fellen31 requested review from Schmytzi, jemten and peterpru November 6, 2024 09:59

Schmytzi requested changes Nov 7, 2024

View reviewed changes

fellen31 and others added 2 commits November 7, 2024 16:19

Update CHANGELOG.md

ed528f4

Co-authored-by: Daniel Schmitz <[email protected]>

Update CHANGELOG.md

c5facbd

Co-authored-by: Daniel Schmitz <[email protected]>

Merge remote-tracking branch 'upstream/dev' into rename-parameters

04cff8a

fellen31 force-pushed the rename-parameters branch 6 times, most recently from f6644aa to ea1de63 Compare November 14, 2024 09:54

review suggestions

a5bd0b3

fellen31 force-pushed the rename-parameters branch from ea1de63 to a5bd0b3 Compare November 14, 2024 09:58

fix CHANGELOG

d9d7187

Schmytzi reviewed Nov 14, 2024

View reviewed changes

nextflow.config Outdated Show resolved Hide resolved

nextflow.config Outdated Show resolved Hide resolved

nextflow.config Outdated Show resolved Hide resolved

fellen31 and others added 3 commits November 15, 2024 08:44

Update nextflow.config

e219500

Co-authored-by: Daniel Schmitz <[email protected]>

Update nextflow.config

3a1ed9a

Co-authored-by: Daniel Schmitz <[email protected]>

Update nextflow.config

47bf777

Co-authored-by: Daniel Schmitz <[email protected]>

Merge branch 'dev' into rename-parameters

fd59653

jemten approved these changes Nov 18, 2024

View reviewed changes

Schmytzi approved these changes Nov 18, 2024

View reviewed changes

fellen31 merged commit 4817e1f into genomic-medicine-sweden:dev Nov 18, 2024
23 checks passed

fellen31 deleted the rename-parameters branch November 18, 2024 10:18

Schmytzi mentioned this pull request Dec 10, 2024

Release 0.4.0 #514

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename input file parameters #488

Rename input file parameters #488

fellen31 commented Nov 6, 2024 •

edited

Loading

Schmytzi left a comment

Schmytzi Nov 7, 2024

Schmytzi Nov 7, 2024

Schmytzi Nov 7, 2024

fellen31 Nov 11, 2024

Schmytzi Nov 7, 2024

fellen31 Nov 11, 2024

Schmytzi Nov 7, 2024

Schmytzi Nov 7, 2024

Schmytzi Nov 7, 2024

Schmytzi commented Nov 7, 2024

fellen31 commented Nov 11, 2024

fellen31 commented Nov 11, 2024

Schmytzi commented Nov 13, 2024

fellen31 commented Nov 14, 2024

Schmytzi left a comment

fellen31 commented Nov 15, 2024

fellen31 commented Nov 18, 2024

jemten left a comment

Schmytzi left a comment

	\| `bed` \| A BED file with regions of interest, used to limit short variant calling. \| `string` \| \| \| \|
	\| `target_bed` \| A BED file with regions of interest, used to limit short variant calling. \| `string` \| \| \| \|

	- Limit SNV calling to regions in BED file (`--bed`).
	- Limit SNV calling to regions in BED file (`--target_bed`).

Rename input file parameters #488

Rename input file parameters #488

Conversation

fellen31 commented Nov 6, 2024 • edited Loading

PR checklist

Schmytzi 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

Schmytzi commented Nov 7, 2024

fellen31 commented Nov 11, 2024

fellen31 commented Nov 11, 2024

Schmytzi commented Nov 13, 2024

fellen31 commented Nov 14, 2024

Schmytzi left a comment

Choose a reason for hiding this comment

fellen31 commented Nov 15, 2024

fellen31 commented Nov 18, 2024

jemten left a comment

Choose a reason for hiding this comment

Schmytzi left a comment

Choose a reason for hiding this comment

fellen31 commented Nov 6, 2024 •

edited

Loading