-
Notifications
You must be signed in to change notification settings - Fork 718
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 RSEM #389
Add RSEM #389
Conversation
Merged the template already today, so if you wait for around 30 mins, at least the linting should be fine then... |
Merge #394 first. |
No sample names were included.
bin/scrape_software_versions.py
Outdated
results['Qualimap'] = '<span style="color:#999999;\">N/A</span>' | ||
results['MultiQC'] = '<span style="color:#999999;\">N/A</span>' | ||
results["nf-core/rnaseq"] = '<span style="color:#999999;">N/A</span>' | ||
results["Nextflow"] = '<span style="color:#999999;">N/A</span>' |
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.
@grst , I'm not sure that these editor-config i.e. '
to "
related changes are necessary. Or perhaps we are adapting this as a convention throughout?
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.
We don't usually enforce this kind of thing (except for maybe tabs/spaces). I wrote this code originally I think and I prefer single quotes, everyone has their own preference 😉 Generally I think it's nicest to just stick with whatever is already there though, saves a massive diff and means the line history is fair.
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.
Though here it looks like the backslashes need removing in the span, so it doesn't really matter.
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.
Seems like I didn't pay attention to my black.
I agree that diffs should be minimal and will revert this (except for the superfluous backslashes I guess)
* STEP 11 - Transcriptome quantification with Salmon | ||
* STEP 15 - Transcriptome quantification with Salmon |
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.
This is a good idea to divide the codebase in steps, however, these tend to add maintainence as well and eventuallly become out of sync. Maybe we could start relying on NOTE
or simply STEP
for this purpose?
Better to wait for a more experienced reviewer
skip_rsem = params.skip_rsem | ||
if (!params.skipAlignment && !params.skip_rsem && params.aligner != "star") { | ||
skip_rsem = true | ||
println "RSEM only works with STAR. Disabling RSEM." |
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.
According to the release notes for RSEM 1.3.3 (https://deweylab.github.io/RSEM/updates.html) there is now a hisat option available.
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 wasn't aware of that. Would indeed be a nice addition.
However I won't have time to look into this any time soon.
Therefore, I opt for merging it now, and adding RSEM+Hisat support in a second PR.
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.
Noted - merging this PR now.
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.
Looks good to merge
This PR adds RSEM to the pipeline (Fixes #70).
Notes:
I downgradedsalmon
to0.14.2
because the env could not be resolved with1.0.0
even before adding rsemLinting fails already for thedev
branch. I didn't attempt to fix it, but don't believe I introduce additional lint.PR checklist
dev
rather thanmaster
nextflow run . -profile test,docker
).nf-core lint .
).docs
is updatedCHANGELOG.md
is updatedREADME.md
is updatedLearn more about contributing: https://github.com/nf-core/rnaseq/tree/master/.github/CONTRIBUTING.md