-
Notifications
You must be signed in to change notification settings - Fork 127
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
Bug in multiqc / mirTop broke #137
Comments
Not sure sorry, I guess the output is different for some reason? Not seen it before, need to see the tool output. These things are usually fairly easy to patch though. |
Hi @ewels I have added some fake data (that resembles the original data very much) to add a testcase for @lpantano might also be interested in seeing this -- it looks like the |
oh, I didn't think about not having that for all samples...that seems weird to be honest, related to the data itself...no sequences matching the references seems weird. Thank for spotting this. |
Considering that we likely need to address this upstream either in mirtop to enforce this is always there in the output or in the multiqc module - can we potentially add a |
Yes, might be a good solution for now. Should we skip the whole process or maybe only multiqc i.e. --skip_mirtop_multiqc ? |
I think the quantification is useful, I would prefer to skip the QC of multiqc in this case only if it were my data. |
I fixed that in mirtop, waiting for bioconda to bump the version, and then we need to update the docker container, that I forgot how to do it, so If somebody knows how, happy to get help here :) |
You mean the mulled container here ? I can take care of it if you are referring to this |
Here is the PR in the biocontainers multi-package containers repository BioContainers/multi-package-containers#2150 |
Here is the PR with the updated container #143 |
All fine now 👍🏻 |
Hmm, unfortunately still an issue. I've had more projects with the same error :-( |
It still looks like some samples are missing the same entry, e.g. |
I'm getting the same problem, looks like same cause; smrnaseq pipeline failing on same mirTop parsing error, rest of the pipeline completes. Can you recommend a workaround, is there a way to suppress multiqc parsing of mirTop in the pipeline or should I just run multiqc separately? |
I think the problem is, that the MultiQC module assumes certain keys to be present in the mirtop output - however mirtop does not always produce these, causing the MultiQC module to crash as it can't find these for all samples when computing metrics. We hoped that the fix @lpantano had made in mirtop 0.4.25 fixed this, but apparently not :-( Currently not really a workaround - skipping multiqc and running it outside means you will not have a nice report which is also not too nice. |
Is there an issue / PR already for this on the MultiQC repo? Should be fairly easy to fix. |
I don't see one, want me to open it? |
Yes please 👍🏻 Then maybe we can put @ErikDanielsson on the job over a coffee break 😅 |
Bug opened MultiQC/MultiQC#1712 |
Maybe a good idea yeah to fix this in MultiQC, although that means we have to wait for MultiQC V1.13 to fix this 😢 |
Hi, |
below is the gff file from the mirtop output in the results directory of the nf-core/smrna output from the run where the multiqc parsing of the mirtop output failed. It's mostly zero counts because it was a bad configuration, rerunning to see if multiqc succeeds with the proper config |
fwiw: my issue was due to a misconfiguration leading to samples with low or zero mapping. Fixing the configuration fixed the multiQC/mirtop problem for me. This is probably still a multiQC issue that should be fixed eventually as a lack of any mapped miRNA is a reasonable edge case. |
Yeah, MultiQC should never crash with a traceback like that. For starters it means that all subsequent logs from that tool will be ignored, so it should definitely be fixed 👍🏻 |
Ok @ErikDanielsson should have fixed it in MultiQC, just merged to If you want this quickly you can do a Bioconda release of dev if you like. People have done that in the past. |
I'll do that then, should be fine to do in such a case and then upgrade in smrnaseq once we have 1.13 again 🥳 |
PR in modules is open nf-core/modules#1814 , once this is merged, can upgrade the module for MultiQC here in the repository and we're fine again 👍🏻 |
MultiQC 1.13 is released, as such this should be fixable by upgrading to the MQC 1.13 module |
Had this unfortunately with the
dev
branch:Not sure what is causing this, didN#t check the module so far.
@ewels any idea?
The text was updated successfully, but these errors were encountered: