Closed lpantano closed 2 months ago
master
branch :x:base
to dev
Hi @lpantano,
It looks like this pull-request is has been made against the nf-core/smrnaseq master
branch.
The master
branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master
are only allowed if they come from the nf-core/smrnaseq dev
branch.
You do not need to close this PR, you can change the target branch to dev
by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.
Thanks again for your contribution!
nf-core lint
overall result: Passed :white_check_mark: :warning:Posted for pipeline commit 7a15447
+| ✅ 217 tests passed |+
#| ❔ 1 tests were ignored |#
!| ❗ 3 tests had warnings |!
Could you upload the testdata and prepare a testcase for this already? That way we could test more easily if the results match expectations, even when trying to solve this without a local module 👍🏻
sounds good, I will do that ASAP, I checked that sequences were matching reference miRNA, so that is good. I will ping again when is ready.
One idea would be to run the fastp module multiple times and use the stageIn directive to copy instead of symlink. That should be possible via the modules.config without having to worry about the data being changed.
I added the test data, not running in ci right now, but we can add it if we want. With this subsampled test data, the majority of the reads map to reference, if you don't running with next flex then all are isomiRs, because of the lack of trimming. So, I think is working.
Ok, I will close this later and open an issue with the propose strategy and see if somebody has time to do it at some point. I will refer to this branch meanwhile for people that has this data to get something working. Thanks. On Sep 5, 2024 at 09:23 -0400, Alexander Peltzer @.***>, wrote:
@apeltzer commented on this pull request. In workflows/smrnaseq.nf:
@@ -7,6 +7,7 @@ include { CAT_FASTQ } from '../modules/nf-core/cat/fastq/ include { CONTAMINANT_FILTER } from '../subworkflows/local/contaminant_filter' include { FASTQC } from '../modules/nf-core/fastqc/main' include { FASTQ_FASTQC_UMITOOLS_FASTP } from '../subworkflows/nf-core/fastq_fastqc_umitools_fastp' +include { FASTP3 } from '../modules/local/trim3p.nf' @nschcolnicov / @atrigila and myself just discussed it again and it should work the way @nschcolnicov just mentioned and also supplying the stageInMode: 'copy' in the modules.config for the FASTP3 then. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
I think we can just use your branch and add the fix, we can likely also help with that to try if the strategy works indeed. The main fix will stay untouched - its just not using a local module to achieve the same thing :)
Ran the pipeline like this now:
nextflow run . -profile docker,nextflex --outdir nextflex --input https://raw.githubusercontent.com/nf-core/test-datasets/smrnaseq/samplesheet/v2.0/samplesheet_test_nextflex.csv --
genome 'GRCh37' --mirtrace_species 'hsa'
@apeltzer , isn't that command need the -profile docker,nextflex
?
Absolutely, added that in
@lpantano @apeltzer @atrigila Should be good to go now, please review.
I will run it later (give me a couple of hours) to check results, and make sure we have the right trimming. I am sure it is right, but good to double check, thanks!
Sounds good - we could also add a test 👍
Ah sorry there is one, let's test if trimming looks good and then merge if ok
yes, I will mainly do that, need to look a little closely, mainly checking that plot in multiqc looks like that
When I ran:
nextflow run main.nf -profile docker,test_nextflex --outdir
Mirtop doesn't run anymore. It should be after NFCORE_SMRNASEQ:MIRNA_QUANT:BOWTIE_MAP_MATURE
. Are you getting Mirtop running with this?
Otherwise, the trimming is working. If we fix that TODO it would be great, right now is skipping the miRNA quantification if people set that parameter.
This line
is making not to setup the ch_mirna_gtf, when the user gives
param.mirna_gtf
, I don't think that is what we want right? it is not related to this PR, actually affecting everything.There is a comment actually there, that we should implement:
//TODO for ch_mirna_gtf, shouldn't it try to build a channel.fromPath with params.mirna_gtf, if true? (instead of setting it to empty). Is this parameter used for non mirgenedb runs?
Thanks for looking into this, I agree, it doesnt look right
@lpantano I think the change introduced in the last commit fixed this issue, as I can now see that mirtop run. Would you mind checking from your side? I can then focus on updating the CI tests.
yes, that is true. That change makes mirtop to run.
please, somebody go a merge this!!!!!!!!!!!!!!!!! what a way to end the week :)
This PR is trying to add the right 3 end trimming after adapter removal for next flex protocol, where some NT needs to be removed from the end, but after adapter removal.
I couldn't figure out a way to reuse the fastp twice in row because the symlinks input/outputs, so if somebody has an idea, happy to implement, for now I added it as local module to run only for next flex.
I have test data to add, if we all are ok with this approach.
PR checklist
nf-core lint
).nf-test test main.nf.test -profile test,docker
).nextflow run . -profile debug,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).