-
Notifications
You must be signed in to change notification settings - Fork 133
Enhanced BLAST results filtering #434
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
Conversation
|
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.
Some smaller suggestions.
CHANGELOG.md
Outdated
@@ -36,6 +38,8 @@ Thank you to everyone else that has contributed by reporting bugs, enhancements | |||
| | `--freyja_lineages` | | |||
| | `--skip_freyja_boot` | | |||
| | `--additional_annotation` | | |||
| | `--min_contig_length` | | |||
| | `--min_perc_cgaligned` | |
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'm not certain this variable is very descriptive. What about --min_perc_contig_aligned
or --blast_perc_aligned
or something else?
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 named it this way because the column is %cgAligned
. But I can name it other way.
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.
In my opinion, the %cgAligned will call for questions if you see this for the first time.
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.
mmmm I think I'll go with --min_perc_contig_aligned
and write a more exhaustive description of the BLAST headers in the documentation. The %cgAligned
column has always been there, but I think it is not properly described. Thanks!
publishDir = [ | ||
path: { "${params.outdir}/assembly/spades/${params.spades_mode}/blastn" }, | ||
mode: params.publish_dir_mode, | ||
saveAs: { filename -> filename.equals('versions.yml') ? null : filename } | ||
enabled: false |
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.
It might not always be interesting to not publish this? What about an additional variable that controls this --save_blast_unfiltered
?
If you agree make sure to keep ${meta.id}.filter.blastn
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.
Unfiltered blast is always saved, but it does it in the other process, here is only unfiltered blast with no header.
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.
It is generated inside modules/local/filter_blastn.nf
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.
Aaah sorry! I missed those 2 new lines :D
Co-authored-by: Joon Klaps <61584065+Joon-Klaps@users.noreply.github.com>
Co-authored-by: Joon Klaps <61584065+Joon-Klaps@users.noreply.github.com>
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.
LGTM!
Fixed issue #378:
.filter.blastn.txt
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).