-
Notifications
You must be signed in to change notification settings - Fork 4
add long_phase supplementary alignment tag to extended args #163
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
base: dev
Are you sure you want to change the base?
Changes from 5 commits
c14136d
14b14c7
c0e3ab2
af57f20
f4e7cc0
bb3199d
be64ce5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,22 +68,24 @@ | |||||||
| "type": "object", | ||||||||
| "properties": { | ||||||||
| "germline_var_keep": { | ||||||||
| "type": "array", | ||||||||
| "type": "string", | ||||||||
| "description": "List of germline variant callers to use. Must include at least one of [deepvariant, clair].", | ||||||||
| "items": { | ||||||||
| "type": "string", | ||||||||
| "enum": ["deepvariant", "clair"] | ||||||||
| }, | ||||||||
| "minItems": 1 | ||||||||
| "minItems": 1, | ||||||||
| "default": "['deepvariant', 'clair']" | ||||||||
| }, | ||||||||
|
Comment on lines
75
to
79
|
||||||||
| "somatic_var_keep": { | ||||||||
| "type": "array", | ||||||||
| "type": "string", | ||||||||
| "description": "List of somatic variant callers to use. Must include at least one of [deepsomatic, clair].", | ||||||||
| "items": { | ||||||||
| "type": "string", | ||||||||
| "enum": ["deepsomatic", "clair"] | ||||||||
| }, | ||||||||
| "minItems": 1 | ||||||||
| "minItems": 1, | ||||||||
| "default": "['deepsomatic', 'clair']" | ||||||||
|
||||||||
| "default": "['deepsomatic', 'clair']" | |
| "default": ["deepsomatic", "clair"] |
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue for somatic_var_keep: the workflow expects a List (uses .contains() / .size() patterns), but the schema now declares it as string with items/minItems and a stringified list default. This makes the schema inconsistent/invalid and can lead to runtime misbehavior if users supply a string. Please keep it as type: "array" with an enum-constrained items, and set default to a JSON array like ["deepsomatic","clair"].
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
germline_var_keep and somatic_var_keep are typed as arrays, but their default values are currently JSON strings (e.g. "['deepvariant', 'clair']"). In JSON Schema, default should match the declared type; this will break schema validation / generated docs. Set these defaults to actual JSON arrays (e.g. ["deepvariant", "clair"]).
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
longphase_options is added under $defs, but it isn't referenced in the top-level allOf list. As a result, longphase_tag_supplementary defined there won't be part of the effective schema grouping/metadata. Add a $ref to #/$defs/longphase_options in allOf (and avoid duplicating the param elsewhere) so it shows up correctly in the schema-driven UI/docs.
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR title/description indicate this is only about adding a Longphase supplementary-alignment haplotag option, but this PR also changes schema defaults for *_var_keep, adds new schema properties for generate_gvcf / skip_modkit, and updates the documented default for severus_minsupport. Please update the PR description (or split into separate PRs) so reviewers understand the full scope and rationale for these additional changes.
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of boolean parameters had their default: false removed from the schema (e.g. all skip_* flags, use_gpu, plus others elsewhere like igenomes_ignore, download_vep_cache, ascat_pdf_plots, and generic help/version flags). Even though JSON Schema default is informational, nf-core tooling/docs and UI generation typically rely on it to display correct defaults and keep the schema aligned with nextflow.config. Please restore the default values (at least for the params that are explicitly defaulted in nextflow.config).
Copilot
AI
Apr 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top-level properties.generate_gvcf is missing a description/help_text/default and is not grouped with the other small variant calling options in $defs. This makes the schema inconsistent and may cause nf-core schema docs / UI grouping to omit or misplace the parameter metadata.
| "generate_gvcf": { | |
| "type": "boolean" | |
| }, |
Copilot
AI
Apr 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top-level properties.skip_modkit is defined without description/help_text/default and outside the existing skip_options group. To keep schema validation + rendered docs consistent, this should be defined alongside the other skip parameters (with matching metadata) rather than as a bare root property.
| }, | |
| "skip_modkit": { | |
| "type": "boolean" |
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new top-level properties block defines generate_gvcf, longphase_tag_supplementary, and skip_modkit with only type, dropping their descriptions/defaults and bypassing the existing $defs grouping pattern used elsewhere in this schema. Instead, define these parameters in the appropriate $defs sections (e.g., generate_gvcf in small-variant options, skip_modkit in skip options, longphase_tag_supplementary in longphase options) and reference those groups from allOf, removing this partial top-level properties block.
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new top-level properties entries for generate_gvcf and skip_modkit are incomplete (no description/default) and also bypass the existing parameter grouping via $defs/allOf (e.g. skip_modkit should live under #/$defs/skip_options). This will make schema-generated docs/UI inconsistent with docs/usage.md and other params. Consider moving these definitions into the appropriate $defs sections and adding description (and default if the schema is intended to carry defaults).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
germline_var_keepnow allowstype: "string", but the allowed values are only constrained viaitems.enum(which applies only when the value is an array). As a result, any arbitrary string would validate successfully and bypass the intended[deepvariant, clair]restriction. Consider usingoneOf/anyOfto separately validate the string form (e.g., enum/pattern) vs the array form, or keep this as an array-only parameter.