Skip to content
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

Division by Zero in Sambamba Markdup #1654

Closed
epruesse opened this issue Mar 25, 2022 · 5 comments
Closed

Division by Zero in Sambamba Markdup #1654

epruesse opened this issue Mar 25, 2022 · 5 comments
Assignees
Labels
bug: module Bug in a MultiQC module

Comments

@epruesse
Copy link
Contributor

epruesse commented Mar 25, 2022

Description of bug

In file sambamba/sambamba.py line 93:

https://github.com/ewels/MultiQC/blob/45ce9ae0160c38e250e3e0e4a9335adc5af26b6b/multiqc/modules/sambamba/markdup.py#L93

A simple try/except should do it. May want to check every place in every module divisions occur. If you have a division, you'll eventually have a division by zero...

File that triggers the error

Simple all empty (previously filtered to nothing) file:

finding positions of the duplicate reads in the file...
  sorted 0 end pairs
     and 0 single ends (among them 0 unmatched pairs)
  collecting indices of duplicate reads...   done in 0 ms
  found 0 duplicates
collected list of positions in 0 min 0 sec
removing duplicates...
collected list of positions in 0 min 0 sec
@ewels ewels added the bug: module Bug in a MultiQC module label Apr 4, 2022
@aidaanva
Copy link
Contributor

aidaanva commented Apr 25, 2022

Hi,
I also had the same error running nf-core/eager and affecting MALT, in this case there was one sample with 0 reads assigned.

The error looks like this:

MultiQC 🔍 | v1.11

|           multiqc | MultiQC Version v1.12 now available!
|           multiqc | Only using modules: adapterRemoval, bowtie2, custom_content, damageprofiler, dedup, fastp, fastqc, gatk, kraken, malt, mtnucratio, multivcfanalyzer, picard, preseq, qualimap, samtools, sexdeterrmine, hops, bcftools
|           multiqc | Search path : /mnt/archgen/MICROSCOPE/pathogen_screening/2021-03-09-keltenfuersten_BW/fullbacetvir2017/work/5c/a5b3dca9f4b85354e0b9529b211141/multiqc_config.yaml
|           multiqc | Search path : /mnt/archgen/MICROSCOPE/pathogen_screening/2021-03-09-keltenfuersten_BW/fullbacetvir2017/work/5c/a5b3dca9f4b85354e0b9529b211141
|         searching | ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 1006/1006  
╭────────────────── Oops! The 'malt' MultiQC module broke... ──────────────────╮
│ Please copy this log and report it at                                        │
│ https://github.com/ewels/MultiQC/issues                                      │
│ Please attach a file that triggers the error. The last file found was:       │
│ ./malt/malt.log                                                              │
│                                                                              │
│ Traceback (most recent call last):                                           │
│   File "/opt/conda/envs/nf-core-eager-2.4.2/lib/python3.9/site-packages/mult │
│     output = mod()                                                           │
│   File "/opt/conda/envs/nf-core-eager-2.4.2/lib/python3.9/site-packages/mult │
│     self.parse_logs(f)                                                       │
│   File "/opt/conda/envs/nf-core-eager-2.4.2/lib/python3.9/site-packages/mult │
│     float(self.malt_data[s_name]["Assig. Taxonomy"])                         │
│ ZeroDivisionError: float division by zero                                    │
│                                                                              │
╰──────────────────────────────────────────────────────────────────────────────╯
|              hops | Found 58 samples
|          samtools | Found 62 flagstat reports
|          samtools | Found 62 flagstat reports
|    custom_content | software_versions: Found 1 sample (html)
|    custom_content | nf-core-eager-summary: Found 1 sample (html)
|           multiqc | Compressing plot data
|           multiqc | Report      : multiqc_report.html
|           multiqc | Data        : multiqc_data
|           multiqc | Plots       : multiqc_plots
|           multiqc | MultiQC complete

@ewels would you advise to open a separate issue?

I think @epruesse is right that this should apply to all divisions.

@ewels
Copy link
Member

ewels commented May 1, 2022

@aidaanva please make a new issue for that as it's a different module.

I agree that these errors are very common, but going through all code for all modules is a mammoth task. It's not a trivial find + replace as each case needs bespoke handling. For example should it use None or 0 in its place? Also when fixed they very often lead to downstream errors where later parts of the code also expect totals to be non-zero. This is why example data is so important for testing when fixing these bugs.

@aidaanva
Copy link
Contributor

@ewels sorry it took so long to reply. It makes sense. I have now create a new issue: #1683

@gartician
Copy link
Contributor

I can elect to follow up in this since I made the Sambamba markdup module. Can I assign myself to this issue? And when it's fixed, should I initiate a new PR for this fix?

@ewels
Copy link
Member

ewels commented May 25, 2022

That would be brilliant @gartician - yes please! Thanks!

ewels added a commit that referenced this issue Sep 9, 2022
fix zero division error in sambamba markdup module #1654
@ewels ewels closed this as completed Sep 9, 2022
bnbowman pushed a commit to bnbowman/MultiQC that referenced this issue Dec 20, 2022
* Try prefixing analysis dirs

* Update CHANGELOG.md

* tried to add conditional execution to actions

* Update CHANGELOG.md

* Add dependency statement

* only overwrite id when not set

* changelog

* always set c_id

* Revert setting force_interactive flag for rich with --no-ansi

* Don't force terminal escape codes for the progress bar

* Extend kallisto module regex to recognize newer output

I've noticed multiqc (v 1.12) didn't recognise some kallisto output (I'm using kb_python 0.26.4).
Having digged a bit, it seems to not work with more recent kallisto output, i.e. this snippet taken from here:
https://github.com/pachterlab/GRNP_2020/blob/daed9c2f204f1c3f6ee0e864c3db93b0baadfc8a/notebooks/FASTQ_processing/ProcessPBMC_NG.ipynb
```
[index] k-mer length: 31
[index] number of targets: 187,626
[index] number of k-mers: 108,619,921
tcmalloc: large alloc 3221225472 bytes == 0x556459b7e000 @  0x7feac4ab5887 0x556458814ad2 0x55645880d061 0x5564587e1372 0x7feac3935bf7 0x5564587e60da
[index] number of equivalence classes: 752,021
[quant] will process sample 1: A_R1.gz
                               A_R2.gz
[quant] will process sample 2: B_R1.gz
                               B_R2.gz
[quant] finding pseudoalignments for the reads ... done
[quant] processed 170,526,037 reads, 98,632,205 reads pseudoaligned
```
Turns out multiqc only looks for `pair|file` but not sample. Replacing sample for file did do the trick, hence I suggest to add `sample` in the regex pattern.
I haven't tested this, but it should work now.

Here is the code which generates this output:
https://github.com/pachterlab/kallisto/blob/83bde908c403ea4014b5092a243e5c7240f48dd5/src/ProcessReads.cpp#L235

This is the commit which introduced it (already in 2018, so not sure why this hasn't been caught yet)
pachterlab/kallisto@62e9464

* Replace logger.hasHandlers() with logger.handlers

There are cases where configuring logging results in logger.handlers being empty but logger.hasHandlers() returns True: MultiQC#1643

Since the block modified removes based on logger.handlers, the condition to enter the block should check logger.handlers rather than logger.hasHandlers()

* Added description of changes for pull request

* Document 'no_version_check' config option

* Docs tweak

* Fix kwargs for MultiQC plugins

* New config option 'custom_table_header_config'

* Run black

* Update adapterRemoval.py

Returns actual proportion of reads that were collapsed and discarded

* Black format

* Black format

* BlackPython

* Fix chart labels and titles

* Fix chart labels and titles

* Add columns to stats table

Add columns with proportion of collapsed/discarded reads to the general stats table

* Add Columns - Fix format

* Changelog

* Fixed bug when other fields also have a "-" instead of an integer.

* Updated CHANGELOG

* Fixed typos

* Fixed format typo

* Fixed format typo

* Nanostat: Remove HTML escaping

Jinja2 escape() function removed in jinja2 v3.10

I don't think that this escaping should be required. I can't see any effect in the report when I remove it anyway.

* Changelog

* Changing 0 to None

* Skip fields with `-`

* Pangolin 4.0 compatability

Recently pangolin has been updated to version 4.0 and this changes the output CSV file - see: https://github.com/cov-lineages/pangolin/releases/tag/v4.0

This causes the module to fail in its current state as row['qc_status'] already exists and the current replacement triggers a key error by searching for row['status'] which no longer exists. Thanks to @alexomics for tracking down the issue.

* Don't duplicate custom-content section descriptions.

Fixed edge-case bug in custom content where a `description` that doesn't terminate in `.` gave duplicate section descriptions.

* Changelog

* Tidied the verbose log, added summaries for skipped search files to debug log

* Allow sorting of table columns with text contents

* update changelog

* optimize linegraph category comparison

* Somalier: division by zero in sex ploidy plot

* Changelog

* Add time zone

* Update changelog

* Fix typo in bcl2fastq.py

* Handle too long and low complexity

* update changelog

* fix zero division error in sambamba markdup module

* black formatting

* update CHANGELOG.md to address MultiQC#1654

* bclconvert checks RunInfo xml if reads are singleend or pairedend and sets clusterlength appropriately. resolves MultiQC#1697

* Added CITATION.cff file for standardized citations

* fixed formatting of url

* fixed citation formatting

* Run prettier

* Fix module crashing due to missing field in report

* Fix bug where module wouldn't run if all content was within a MultiQC config file

Fixes MultiQC#1686

* nanostat: add check for quality scores

* update CHANGELOG.md

* update CHANGELOG.md

* Custom content: Fix crash when 'info' isn't set

Closes MultiQC#1688

* Added nix flake support

* Update docs/installation.md

Co-authored-by: Phil Ewels <phil@seqera.io>

* Fix zero division error

* Update fastqc.py

* Update fastqc.py

* fix format

* add change log

* fix doc ref

* Don't need Prettier _and_ markdown/yamllint CI

* Just capture the ValueError

* Rich-codex screenshot in the readme

* Corrected 'outdir' flag

Missing a dash for the flag to work.

* Clean up clean_img_paths

* Generate new screengrabs with rich-codex

* Add samtools flagstat column '% Read Mapped'

* update samtools flagstat changelog

* Added try,except for divisions to avoid division by 0 errors

* added the fixing of malt in the change log

* report median read length for fastqc

* add after filtering total reads to general stats table

* GitHub Actions: Tweet about new releases

* Bump to v1.13 for release

* rich-codex screenshots: Manual only, skip git checks

* Generate new screengrabs with rich-codex

* Fix changelog date

* Bump to v1.14dev

* Custom content: Render report even if there's only general stats there

See MultiQC#1756

* Bugfix: Make `config.data_format` work again

* Bump minimum version of Jinja2 to `>=3.0.0`

Closes MultiQC#1642

* Disable search progress bar if running with `--quiet` or `--no-ansi`

Closes MultiQC#1638

* Attempt to cooerce line / scatter x-axes into floats so as not to lose labels

See MultiQC#1242

* Use row 1 as x-axis labels if no sample name.

Closes MultiQC#1242

* Malt: Move changelog up to new version

* Merge changelog up

* Use OrderedDict instead of 'placement'

* Add code comment

* Add CI testing for Python 3.10 and 3.11

* Fix typo

* Quotes so it's 3.10 and not 3.1

* 3.11-dev

* Remove 3.11-dev for Windows

* Move merge markers GHA into lint workflow file

* Shorter job name

* Be more selective about when slow MultiQC test runs fire

- Master only for push event
- Don't run if only docs / markdown

* Run isort

* Remove py2 'from __future__ import print_function'

* Add GitHub actions CI for isort

* Changelog

* Remove all py2 'from __future__ imports'

* Tweak some imports

* Changelog

* added setuptools to flake

* rm emtpy bcftools stats variant depths plot

* moved changelog comment

* adjusted PR num

* fix duplicate heatmap for kraken

* changelog

* use None instead

* First commit of pre-commit

* Comment out all the tests that don't yet work

* Update gene_body_coverage.py

Using a normalized coverage to make genebody coverage plot ( similar to the method used by RSeQC). Us the formula 'norm_cov = ( cov - min(cov ) / ( max(cov) - min(cov) )' to compute normalized coverage

* Update gene_body_coverage.py

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Move changelog entry

* Test for Python 3.11 now that the official release is out

* CI: Use new version of actions/checkout to avoid Node.js depreciation warning

* Remove sample and chromosome before converting to int

This fixes issue-1793

* Remove filtered samples from general stats table

This fixes MultiQC#1780

* Update changelog

* Add additional entries for qualimap when region stats present

* fastp: use passed filter reads instead of after filter total reads

Signed-off-by: Josh Chorlton <jchorlton@gmail.com>

* bclconvert now handles different r1 and r2 lengths instead of assuming they are the same

* updated CHANGELOG.md

* update bustools

* Update CHANGELOG.md

* Remove changelog entry

* Move changelog to entry to correct place

* Fix changelog

* Kraken: Improve heatmap config

* Apply suggestions from code review

Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

* handle singleindex data

* cleanup

* CHANGELOG.md bclconvert fix issue link typo and note single-index paired-end data handled

* Qualimap BamQC: Refactor to parse regexes per section

Also: Fix randomly aggressive Snippy module parsing bug

* HsMetrics: Allow custom columns in General Stats too

* Replace nested loop with list comprehension when parsing output file headers

* CHANGELOG

* Output headers order preserved and Sample is first value

* Fix ubuntu version in GitHub CI to preserve Py3.6 testing.

Python 3.6, I think your days are numbered..

* add back original avg field

Signed-off-by: Josh Chorlton <jchorlton@gmail.com>

* fixes

Signed-off-by: Josh Chorlton <jchorlton@gmail.com>

* update busco colors

Signed-off-by: Josh Chorlton <jchorlton@gmail.com>

* fix: frontmatter yaml formatting issue

* Update docs to use --cl-config instead of --cl_config

Closes MultiQC#1825

* Update multiqc/modules/fastqc/fastqc.py

Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

* Update multiqc/modules/fastqc/fastqc.py

Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

* Update multiqc/modules/fastqc/fastqc.py

Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

* suggestion

Signed-off-by: Josh Chorlton <jchorlton@gmail.com>
Co-authored-by: Erik Danielsson <danielsson.erik.0@gmail.com>
Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>
Co-authored-by: Ido Tamir <ido.tamir@vbcf.ac.at>
Co-authored-by: seb-mueller <sebm@posteo.de>
Co-authored-by: Jonathan Oribello <Jonathan.d.oribello@gmail.com>
Co-authored-by: NiemannJ <69033839+NiemannJ@users.noreply.github.com>
Co-authored-by: fgvieira <fgarrettvieira@gmail.com>
Co-authored-by: mattloose <matt.loose@nottingham.ac.uk>
Co-authored-by: Josh Chorlton <jchorlton@gmail.com>
Co-authored-by: vladsaveliev <vladislav.savelyev@populationgenomics.org.au>
Co-authored-by: Sam Chorlton <>
Co-authored-by: jethror1 <45037268+jethror1@users.noreply.github.com>
Co-authored-by: Garth Kong <kongga2017@gmail.com>
Co-authored-by: Andrei Seleznev <aseleznev@illumina.com>
Co-authored-by: lew2mz <david.lewis@cchmc.org>
Co-authored-by: Phil Ewels <phil@seqera.io>
Co-authored-by: phue <patrick.huether@imp.ac.at>
Co-authored-by: David Lewis <60514384+IllustratedMan-code@users.noreply.github.com>
Co-authored-by: Chang Y <yech1990@gmail.com>
Co-authored-by: beausoleilmo <beausoleilmo@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jean Mainguy <jean.mainguy@outlook.fr>
Co-authored-by: aidaanva <aida.andrades@gmail.com>
Co-authored-by: SusiJo <susanne.jodoin@gmx.de>
Co-authored-by: Phil Ewels <phil.ewels@seqera.io>
Co-authored-by: TNalpat <thomas.nalpathamkalam@gmail.com>
Co-authored-by: Redmar van den Berg <RedmarvandenBerg@lumc.nl>
Co-authored-by: James Fellows Yates <jfy133@gmail.com>
Co-authored-by: Maarten-vd-Sande <maartenvandersande@hotmail.com>
Co-authored-by: Adam Talbot <adam.talbot@nonacus.com>
Co-authored-by: Oleh Pratsko <olehpratsko@gmail.com>
Co-authored-by: Josh Chorlton <jchorl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: module Bug in a MultiQC module
Projects
None yet
Development

No branches or pull requests

4 participants