nf-test quantify pseudoalignment #1246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR is against the
|
|
Fixes: - a single sample will now work and not raise an error when trying to parse the count data - duplicate transcripts now work with Salmon (previously it dropped one of them)
adamrtalbot
requested review from
maxulysse,
drpatelh and
pinin4fjords
and removed request for
maxulysse
|
@drpatelh this changes the behaviour of Salmon when dealing with duplicate transcripts. Previously, it combined any transcripts with the same sequence. Now, it will keep all transcripts with the same sequence, which will affect mainly alternate haplotypes but I think it's the right™️ option? Want to check if you think this behaviour is OK before we merge. |
19 tasks
subworkflows/local/quantify_pseudo_alignment/tests/main.nf.test
Outdated
Show resolved
Hide resolved
drpatelh
reviewed
Mar 12, 2024
drpatelh
reviewed
Mar 12, 2024
| @@ -39,7 +39,8 @@ process { | |||
| withName: 'SALMON_INDEX' { | |||
| ext.args = { [ | |||
| params.gencode ? '--gencode' : '', | |||
| params.pseudo_aligner_kmer_size ? "-k ${params.pseudo_aligner_kmer_size}": '' | |||
| params.pseudo_aligner_kmer_size ? "-k ${params.pseudo_aligner_kmer_size}": '', | |||
| '--keepDuplicates' | |||
There was a problem hiding this comment.
Not sure what sort of behaviour this is going to have @adamrtalbot so a bit reluctant to add it in without some proper testing. Did we need to add it to fix something else? Otherwise maybe we create an issue outlining why we need it in and do a proper assessment before adding to the pipeline.
There was a problem hiding this comment.
We discovered it while fixing the tests. When you have two identical sequence transcripts, Salmon will just drop one of them. Downstream, the tools for matching transcripts/genes/names causes an error because it's missing some data. This flag disables this behaviour and makes it match STAR-RSEM, Kallisto etc.
@pinin4fjords has fixed as many downstream problems as he can, but silently dropping transcripts feels like the wrong behaviour in the first place.
There was a problem hiding this comment.
Reverted SALMON_INDEX from keeping duplicates, raised this issue to discuss further: #1259
When combined with the fix for the module, the tests should pass now (famous last words).
Changes: - SALMON_INDEX will keep duplicates - summarizedexperiment will handle the missing transcripts - Version numbers checked in QUANTIFY_PSEUDO_ALIGNMENT subworkflow
pinin4fjords
approved these changes
Mar 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Draft PR for an nf-test for QUANTIFY_PSEUDOALIGNEMNT
Problems:
SALMON_QUANTSE_GENE_LENGTH_SCALEDAll fixed!
This changes one global parameter when using Salmon, which means it now keeps duplicate transcripts (i.e. the same sequence, not the same transcript ID).
The rest is pretty straightforward testing.
PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).