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

Adding Manta module, associated pipeline & config changes #27

Merged
merged 23 commits into from
Jun 10, 2021

Conversation

timothyjsanders
Copy link
Contributor

@timothyjsanders timothyjsanders commented May 27, 2021

See outputs in /hot/pipelines/development/slurm/call-gSV/outputs/v3-manta/amini/TWGSAMIN000001-T001-S01

pipeline/call-gSV.nf Outdated Show resolved Hide resolved
pipeline/call-gSV.nf Outdated Show resolved Hide resolved
@timothyjsanders timothyjsanders marked this pull request as ready for review June 7, 2021 19:22
@timothyjsanders timothyjsanders changed the title Adding Manta module, associated pipeline & config changes (draft PR) Adding Manta module, associated pipeline & config changes Jun 7, 2021
@timothyjsanders timothyjsanders requested a review from a team June 8, 2021 16:08
metadata.yaml Outdated Show resolved Hide resolved
* **map-qual:** >= 20 (Applied / Parameterized)
* **pe:** >= 5 (Not yet Applied / Non-parameterized)
* **sr:** >= 5 (Not yet Applied / Non-parameterized)
* **keep_imprecise:** >= true (Not yet Applied / Non-parameterized)


Choose a reason for hiding this comment

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

Maybe we want to say default instead of Non-parameterized?

Copy link
Contributor Author

@timothyjsanders timothyjsanders Jun 9, 2021

Choose a reason for hiding this comment

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

Maybe something like the following? Since there are default params in Delly, but some the user can't change.

**map-qual:** >= 20 (Default / Parameterized)
**pe:** >= 5 (Default / Non-parameterized)
**sr:** >= 5 (Default / Non-parameterized)

Choose a reason for hiding this comment

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

Hmm then we should add the Delly default values? I meant the Delly default values not our default. Also, PE and SR are not independent values here. You can execute delly call, etc to see avilable options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I think I wasn't clear earlier. So a parameter that is listed as "Default / Non-parameterized" (for example pe >= 5) is a Delly default that isn't parameterized in the call-gSV pipeline. The map-qual parameter, on the other hand, has a default value of 20 and is parameterized in the call-gSV pipeline.

Our original intention was to do as you suggest, include the Delly default values, but also to indicate which ones users were able to change. It does seem like our list might be out of date though

Copy link

@tyamaguchi-ucla tyamaguchi-ucla Jun 10, 2021

Choose a reason for hiding this comment

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

Yup, so maybe we can create a table to show - Delly default, call-gSV default? This is related to #6 . This PR is to add Manta so this can be done in a separate PR. Btw, do you know if Manta has the same option to set a PE/SR cut-off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manta doesn't have the same options that Delly does (or at least I don't know where to find them). Perhaps this would be a filtering step after Manta runs? I've added a table in the README with the Delly defaults and whether or not they are available to set in the pipeline.

README.md Outdated

Currently the following filters are applied and or considered for application and parameterization in subsequent releases:
<br>
**Delly**
* **map-qual:** >= 20 (Applied / Parameterized)
* **pe:** >= 5 (Not yet Applied / Non-parameterized)
* **sr:** >= 5 (Not yet Applied / Non-parameterized)
* **keep_imprecise:** >= true (Not yet Applied / Non-parameterized)

Choose a reason for hiding this comment

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

I think Delly keeps imprecise sites as default. So, it's already applied as default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I can clarify some of the wording in the readme to make it more clear what's applied and what the user can modify

README.md Outdated
Comment on lines 71 to 72
* **pe:** >= 5 (Not yet Applied / Non-parameterized)
* **sr:** >= 5 (Not yet Applied / Non-parameterized)
Copy link

@tyamaguchi-ucla tyamaguchi-ucla Jun 9, 2021

Choose a reason for hiding this comment

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

Required PE/SR clique size is 2 as default (PE/SR evidence combined) -z [ --min-clique-size ] arg (=2) min. PE/SR clique size - we may want to increase this default value as we used to do before.

Initial call-gSV runs will be used to detect overall gSV candidates so we could go with stringent cut-offs since our germline cohort is pretty large and then the 2nd discovery (regenotype-gSNP) can be more permissive. Any thoughts? @jmlivingstone @pboutros

Copy link

Choose a reason for hiding this comment

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

I think we'll have use-cases where we want to just run Manta and skip the delly and regenotyping steps (for compute efficiency), so I'd suggest not making the initial discovery too permissive here.

Copy link

@tyamaguchi-ucla tyamaguchi-ucla Jun 10, 2021

Choose a reason for hiding this comment

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

Yeah, that's true. For regenotyping using Delly, we could add confident SV candidates predicted by Manta but I'm not sure if it's easy to incorporate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we parameterize all of the delly call and delly cnv parameters in another PR? We have just a handful of them available for modification by users currently.

Choose a reason for hiding this comment

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

I don't think we need to parameterize everything - Caden resolved this issue by allowing extra arguments. See https://github.com/uclahs-cds/pipeline-call-sSNV/blob/master/pipeline/modules/mutect2-processes.nf

README.md Outdated
| Parameter | Delly default | call-gSV default | Description |
|:------------|:----------|:-------------------------|-------------|
| `svtype` | ALL | | SV type to compute (DEL, INS, DUP, INV, BND, ALL) |
| `map-qual` | 20 | 20 | Minimum paired-end (PE) mapping quality |

Choose a reason for hiding this comment

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

Looks like Delly default is not 20

-q [ --map-qual ] arg (=1) min. paired-end (PE) mapping quality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

@@ -61,13 +61,22 @@ A directed acyclic graph of your pipeline.

Choose a reason for hiding this comment

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

I just noticed that the flow diagram looks outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the pipeline in another PR shortly

@timothyjsanders timothyjsanders merged commit 3e28db0 into main Jun 10, 2021
@timothyjsanders timothyjsanders deleted the ts-manta branch June 10, 2021 22:29
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