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

Add msisensor (#95) #163

Merged
merged 17 commits into from
Mar 20, 2020
Merged

Add msisensor (#95) #163

merged 17 commits into from
Mar 20, 2020

Conversation

davidmasp
Copy link
Contributor

@davidmasp davidmasp commented Mar 18, 2020 •
edited

nf-core/sarek pull request

Description

Adding MSIsensor as a tool to Sarek. (from here #95 )

First nf-core commit so I am a bit clueless of what I am doing. Basically I added code to the pipeline to run msisensor which detects MSI status from NGS tumor/normal pairs. It's divided in 2 steps which I would say fit very nicely in Sarek. I guess this is a WIP PR. Will update further but thought it would be better to submit already.

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!
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: CONTRIBUTING.md

@davidmasp
Copy link
Contributor Author

I did build the environment in conda and I managed to run the nextflow run . -profile test --tools msisensor -resume and it does run but I don't have docker installed rn. I could try to configure it when I have more time though.

@davidmasp
Copy link
Contributor Author

For the test I am not sure what to do, should I just include a test profile like conf/test_msi.config and there add tools msi? I mean, does it also check the output or just if the pipeline runs.

Regarding the output, the tool runs but it detects 0% which makes sense because the test dataset is small so I was also wondering if I need to build a test dataset to get a "supervised" output or just the fact that it runs is enough? I am not sure how I would add microsattellite inestability in a bam file, I guess it's possible though.

maxulysse
maxulysse reviewed Mar 19, 2020
View reviewed changes
main.nf Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 19, 2020
View reviewed changes
main.nf Show resolved Hide resolved
@maxulysse
Copy link
Member

For the test I am not sure what to do, should I just include a test profile like conf/test_msi.config and there add tools msi? I mean, does it also check the output or just if the pipeline runs.

Regarding the output, the tool runs but it detects 0% which makes sense because the test dataset is small so I was also wondering if I need to build a test dataset to get a "supervised" output or just the fact that it runs is enough? I am not sure how I would add microsattellite inestability in a bam file, I guess it's possible though.

For now, let's just try adding MSIsensor to the list here:

sarek/.github/workflows/ci.yml

Line 134 in 52bee46

tool: [Haplotypecaller, Freebayes, Manta, mpileup, Strelka, TIDDIT]

We'll figure something out later.
We have an issue here to make some test data for Mutect2:
#148
I'm hoping that would be a solution for MSIsensor testing as well.

maxulysse
maxulysse reviewed Mar 19, 2020
View reviewed changes
main.nf Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 19, 2020
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR, there's just a couple of things missing, but it's very promising.
I've been willing to add this tool for a long time.
I'm really happy that you did it

@davidmasp
Copy link
Contributor Author

Okay, I think mostly I understand what i need to fill up

@davidmasp
Copy link
Contributor Author

Will update when I have time, thanks!

@davidmasp
Copy link
Contributor Author

For now, let's just try adding MSIsensor to the list here:

sarek/.github/workflows/ci.yml

Line 134 in 52bee46

tool: [Haplotypecaller, Freebayes, Manta, mpileup, Strelka, TIDDIT]

okay, done in 9afaa94

@davidmasp
Copy link
Contributor Author

How should I update the readme?

@maxulysse
Copy link
Member

How should I update the readme?

Add your name to the list of helpful contributors ;-)

@maxulysse
Copy link
Member

But I guess you were speaking about the docs, and not the actual README file.

Basically, I would add a section in Variant Calling for microsatellites.
And then as we do for any other tool, just describe the actual tool (usually, I copy from their readme), and the output files.

@davidmasp
Copy link
Contributor Author

Ah okok!

@davidmasp
Copy link
Contributor Author

so only docs missing, I wanna check what I write is helpful and true so I'll take some time, not much I guess though.

@davidmasp
Copy link
Contributor Author

everything should be more or less done I think just lmk otherwise. The test isn´t passing because it´s not using the new docker built right? I am still not completely sure how this works.

Cheers

maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
main.nf Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
@maxulysse
Copy link
Member

That looks perfect, I just made a couple of more suggestions.
I think it should be MSIsensor according to their GitHub repo, and I prefer having MSIsensor as the output directory, as it's the name of the tool.

@maxulysse
Copy link
Member

I'll push to dockerhub a new dev container containing msisensor and relaunch the tests as well

maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
@davidmasp
Copy link
Contributor Author

davidmasp commented Mar 20, 2020 •
edited

okay great! the MSI directory was to match the VariantCalling directory. As strelka is inside the variantcalling directory for instance. Does make sense? maybe the msisensor could go there too idk. Now is going directly to the results root directory?

@maxulysse
Copy link
Member

Oh, yes, sorry, I don't have my whole head on the code today.
ASCAT results are in the VariantCalling directory as well, so I'll put MSIsensor results there as well for now

maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
docs/output.md Outdated Show resolved Hide resolved
maxulysse
maxulysse reviewed Mar 20, 2020
View reviewed changes
main.nf Outdated Show resolved Hide resolved
@davidmasp
Copy link
Contributor Author

Awesome! now I think all is good no?

@maxulysse
Copy link
Member

maxulysse commented Mar 20, 2020 •
edited

Awesome! now I think all is good no?

Yes!!!
Just waiting for the CI results and we're good 👍

@davidmasp
Copy link
Contributor Author

nice!

@maxulysse maxulysse added this to the 2.6 milestone Mar 20, 2020
@maxulysse maxulysse merged commit c45716b into nf-core:dev Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.6
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@davidmasp @maxulysse