-
Notifications
You must be signed in to change notification settings - Fork 582
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
Fix for 0 division error in fastp module. #1489
Conversation
@ewels one thing to add is that this additional fix causes the warning to be reported twice. Would it be better to add the |
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.
On my phone so a bit tricky to review properly, but looks good - thanks! 👍🏻
Please just add to the log message which sample had zero reads. If the sample makes it through to the report (isn't skipped) and shows zero reads, then no log message is needed.
multiqc/modules/fastp/fastp.py
Outdated
@@ -232,6 +232,8 @@ def parse_fastp_log(self, f): | |||
) * 100.0 | |||
except KeyError: | |||
log.debug("Could not calculate 'pct_adapter': {}".format(f["fn"])) | |||
except ZeroDivisionError: | |||
log.warning("FastQ input file has zero raw reads") |
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.
Could you please log the sample name here?
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.
In fact I'd be inclined to just merge this with the catch / log message above.. What do you think?
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 think it would be best to either report once per sample or report the sample in the message. What is your opinion of adding a pass
instead of logging sample?
Using the data from #1499 and MultiQC
build from 2dd31c1 results in the following:
/// MultiQC 🔍 | v1.12.dev0
| multiqc | Search path : /home/johnny/data/testing/multiqc_fastp_bug/PCRblank.fastp.json
| searching | ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 1/1
| fastp | FastQ input file has zero raw reads
| fastp | FastQ input file has zero raw reads
| fastp | Found 1 reports
| multiqc | Compressing plot data
| multiqc | Report : report/multiqc_report.html
| multiqc | Data : report/multiqc_data
| multiqc | MultiQC complete
which I think is confusing given that it is one sample.
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.
the sample does make it into the report.
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.
Sorry for the delay. Yup, I think that we're saying the same thing here - as it is the error message is a bit confusing (repeated, sample is still there).
So I was suggesting that we log the same name (no longer repeated) or just merge with the statement above (debug so not in the main console output unless verbose, also specifies what is going wrong and the sample name). I think I'll go for the latter if that's ok.
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.
Tweaked in fe471c6
Does this also fix #1499 ? |
Fixes issue where a division by 0 occurs if there are 0 reads in input fastq.
Additional fix on top of #1451 which updates
CHANGELOG.md
in 797674f.fixes #1466
fixes #1444
CHANGELOG.md
has been updated