-
Notifications
You must be signed in to change notification settings - Fork 674
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
MultiQC report: Issues with FastQC #1308
base: dev
Are you sure you want to change the base?
Conversation
|
5a8912a
to
e75b875
Compare
Some progress:
|
Thanks @MatthiasZepper !!
I read through your write-up but was a little unclear as to what is still missing here? |
To copy in @MatthiasZepper's note on this from Slack: I am somewhat stuck with #1308, both because of a lack of time recently and also a lack of ideas. I believed that I fixed 3 of the 4 issues with the 4th, the inconsistent naming of the TrimGalore! output, being somewhat neglectable. However, it turns out that I did not fix the main issue yet. The reports generated by MultiQC when run inside the pipeline and manually on the outdir of the pipeline differ. The manual runs look exactly how I want them, so I thought it should be good, but the pipeline version does not work alike. In the pipeline version, the path_filters in the MultiQC config (workflows/rnaseq/assets/multiqc/multiqc_config.yml) are not applied:
I think that is because the file paths in the ch_multiqc_files are still those to the work dir and to not correspond yet to the final folder structure specified by the publishDir directives when I mix the output into the channel…
… but since I can’t do a proper introspection into the channel (a .view() or .collectFile() completely crashes the pipeline), I don’t know for sure. |
OK, I know the fix @MatthiasZepper, I sorted this in riboseq. The issue is that the file structure is flat by the time it gets to MultiQC. We need to do like:
... and then:
So we're using the |
Thank you so much! That would be fantastic! You should be able to push to the branch since you are a maintainer, but just in case, I have also invited to as a collaborator to my fork! |
@MatthiasZepper OK, committed! Had a quick check and I think this works, though I note that the trimgalore subworkflow doesn't do a post-trim FASTQ, which we might want to address at some point.... Anyway, I'll let you take it home from here :-) |
Thank you so much! I will try my best to finish this quickly now!
Oh, it does. It is just confusing, because TrimGalore! in itself is a wrapper script around cutadapt and FastQC. So FastQC is not run as a Nextflow process but by the TrimGalore Perl script. |
Ahh right, thought I was forgetting something ;-). So there is probably a missing bit to get those outputs prefixed correctly, but you know what to do. |
@MatthiasZepper in case it's impacting on your work, we've noticed that the lastest MultiQC has generated some issues in the workflow. We're looking into it. |
c005701
to
3ad2adf
Compare
I think/hope/wish I am done with this PR. It now fixes 3 out of the 4 issues that were spotted with the TrimGalore! renaming being left. However, I perceive this as a minor issue and think that it could be tackled some when later if needed. |
Great, thanks @MatthiasZepper ! Just to be clear, you don't need an updated MultiQC? |
It did need changes to MultiQC, since the previous version was not working. However, the critical bug was fixed with 1.22.2 and my updates to the umi-tools module were already contained within 1.22.3. Therefore, with this PR, we should now see (re)introduced:
|
…in the General Stats table of MultiQC.
…d 'umi_log' and not 'log'. The meta.yml is wrong.
…onfiguration parameters used in the config were deprecated.
0031778
to
6178e8c
Compare
@@ -124,19 +124,13 @@ line="#id: dupradar | |||
# - color: 'green' | |||
# dash: 'LongDash' | |||
# label: | |||
# style: {color: 'green'} |
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.
Since this is an nf-core module we should really be fixing this in nf-core modules, or via the patch currently on dev (which I just noticed has some config bits I should probably remove)
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.
Fair enough. I did admittedly not consider whether that was a local file or is provided with a module. However, I noticed that @drpatelh also changed that file locally for the pipeline. So I think we would redo that for the module once we tested everything with a new MultiQC version?
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.
Yep, made a patch for that one
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.
Ok, I have now undone all changes and reverted to your file version from when you turned it into a module.
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.
And here is the corresponding draft PR to the modules' repo. I think, we should keep it as draft for now, until we can properly test with the new MultiQC version.
…rshil Patel and me, since they should be fixed upstream in the modules directory. Reset state of file to cc1bf2c.
Hope you don't mind @MatthiasZepper - just illustrating in those last couple of commits what I meant. So use the module in its updated form, but also have a patch to help with updates. I also removed something I added to the patch earlier and which shouldn't have been there, and bumped the module (think it was just Maxime mucking about with stubs) |
This draft PR comprises my current progress towards fixing issue #1303.
It does modify the
publishDir
directives in the FastQC module config such that the reports are consistently published in${params.outdir}/fastqc/raw
and${params.outdir}/fastqc/trim
regardless of the chosen trimmer (TrimGalore!, Fastp), and adapts the custom MultiQC config of the pipeline accordingly.This is, however, not sufficient to fix the issue, because recent versions of MultiQC have a bug that prevents running the same module twice. There are still separate entries and columns in the General Statistics table, but the modules are not shown in the report and navigation bar:
For both screenshots, I ran MultiQC on the output directory of a
test
profile run of this pipeline using the custom profile inworkflows/rnaseq/assets/multiqc/multiqc_config.yml
.It should be stressed that the FastQC module itself works in modern versions, because if the custom config is omitted, it is also shown. But forcing the module to run twice via a custom config seemingly breaks it. Only in the
General Statistics
table, it still works like a charm. Thus, the reports are parsed, but the module output is not displayed in the report.Further issues
In the course of troubleshooting this issue, I discovered more issues that need to be tackled. Help would be greatly appreciated with those:
Inconsistent naming of FastQC output:
For FastP, the file names are retained before and after trimming:
For TrimGalore!, the RAP1_UNINDUCED samples are renamed with a trimmed suffix and the others receive
_val1_
and_val2_
suffixes.Unfortunately, I have no idea why. I have quadruplechecked the
publishDir
directives and can't explain. Help and inspiration needed!Duplicate column is actually shown in the General Statistics table (FIXED!)
According to the config, the duplicate column from FastQC should be hidden in the General Statistics table. However, it is shown. Might be another MultiQC bug or that I just stared myself blind.
umi-tools dedup stats not shown (Fixed)
According to our current
master
/dev
branch config, the umi_tools module is not run. Seeing this, I believed that would be an easy fix for #1277 and added the module in the config. However, no reports are shown. Either the module is broken or the deduplication stats are not channelled to MultiQC. In either way, also no quick solution in sight here.PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).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).