Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add InterProScan to Pipeline and integrate in AMPcombi #428
base: dev
Are you sure you want to change the base?
Add InterProScan to Pipeline and integrate in AMPcombi #428
Changes from 17 commits
86592d9
491f25d
bbd456e
5b5cb3e
d8c5bf2
fd1ef46
b54f1ea
fee3adb
8b44ed5
ed81b0b
0340ba9
7e1f164
5be17ef
b782b54
e58b322
310a0be
8c8aeaf
804da18
186d351
f88fe9c
5d1f4b7
a7e681d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are we sure we want the output in
${params.outdir}/protein_annotation/interproscan/
and not in${params.outdir}/annotation/interproscan/
? I'd prefer the latter, to have it all in one place regardless of DNA (pyrodigal etc.) or protein annotation (interproscan). I think it's more intuitive to search for any annotation results in a single folder.If not, what do you think of renaming the
annotation
output folder tocontig_annotation
?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 other annotation tools in the annotation workflow are annotating CDS which also can be proteins. I would leave the annotation workflow as is because thats the baseline annotation step of the pipeline. This annotation step is more of an accessory annotation to the pipeline if the user wants more information from diff DB and (1) its technically not correct protein_annotation because those are nnot necessary proteins and (2) the plan is that we add more to this workflow (e.g. the functional annotation of those CDS) so protein annotation will no longer be valid.
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 ouput im not sure if its a good idea to add it in the same folder because those are two diff workflows
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.
Okay. No strong opinion from my side. I still don't find the naming super intuitive, but it can be. Or why not
cds_annotation
for the CDS tools (prokka etc.) versusprotein_annotation
(interproscan)?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.
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.
U suggest the opposite i.e. change the default to disable it, then we wont get any output in the tsv like this !?
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.
Hmm no, I changed to enable it by default, i.e. if the param is false (which is default), then activate
--enable-tsv-residue-annot
. Or am I missing some logic of this?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.
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.
Jasmin did u test it with this DB version? I would not change this unless we test it first to make sure that doesnt break anything
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.
No, I didn't. But you're right, then I will download it to our server and test it; would be nice to have a more recent version towards the pipeline release.
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.
I tested the database version 5.72-103.0 and can confirm that all output files contain valid results, i.e.
results/protein_annotation/interproscan/<samplename>_interproscan.faa.tsv
andresults/reports/ampcombi2/Ampcombi_summary.tsv
. In my test case they are even identical. We can go ahead with the new version, so please commit my database update comments before merge :)Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.