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

Update fastqc.py #1725

Merged
merged 7 commits into from
Jul 17, 2022
Merged

Update fastqc.py #1725

merged 7 commits into from
Jul 17, 2022

Conversation

y9c
Copy link
Contributor

@y9c y9c commented Jul 16, 2022

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated
  • There is example tool output for tools in the https://github.com/ewels/MultiQC_TestData repository or attached to this PR
  • Code is tested and works locally (including with --lint flag)
  • docs/README.md is updated with link to below
  • docs/modulename.md is created
  • Everything that can be represented with a plot instead of a table is a plot
  • Report sections have a description and help text (with self.add_section)
  • There aren't any huge tables with > 6 columns (explain reasoning if so)
  • Each table column has a different colour scale to its neighbour, which relates to the data (eg. if high numbers are bad, they're red)
  • Module does not do any significant computational work

multiqc/modules/fastqc/fastqc.py Outdated Show resolved Hide resolved
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, thanks 👍🏻 Just need to fix black code formatting again and add a changelog entry please.

((100.0 - float(pd["total_deduplicated_percentage"])) / 100.0) * pd["Total Sequences"]
)
pdata[s_name]["Unique Reads"] = pd["Total Sequences"] - pdata[s_name]["Duplicate Reads"]
has_dups = True
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit late now sorry, but this would have been a simpler fix 😉

Suggested change
except KeyError:
except (KeyError, ValueError):

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@ewels
Copy link
Member

ewels commented Jul 17, 2022

Hmm, just tested on the example data you provided in #1724 and it does fix the ValueError problem, but I get another new error now:

$ multiqc -f .

  /// MultiQC 🔍 | v1.13.dev0 (f358065)

|           multiqc | Search path : /Users/ewels/GitHub/MultiQC/TestData/data/modules/fastqc/issue_1724
|         searching | ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 1/1
|            fastqc | Found 1 reports
╭───────────────────────────── Oops! The 'fastqc' 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: ./fastqc_data.txt              │
│                                                                                                       │
│ Traceback (most recent call last):                                                                    │
│   File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/multiqc.py", line 654, in run                     │
│     output = mod()                                                                                    │
│   File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/modules/fastqc/fastqc.py", line 121, in __init__  │
│     self.read_count_plot()                                                                            │
│   File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/modules/fastqc/fastqc.py", line 355, in read_coun │
│     plot=bargraph.plot(pdata, pcats, pconfig),                                                        │
│   File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/plots/bargraph.py", line 104, in plot             │
│     if type(cats[0]) is str or type(cats[0]) is unicode:                                              │
│ IndexError: list index out of range                                                                   │
│                                                                                                       │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────╯
|           multiqc | No analysis results found. Cleaning up..
|           multiqc | MultiQC complete

This is why I always insist on having example data - very often it's a game of whack-a-mole with these kinds of errors 😄

@ewels
Copy link
Member

ewels commented Jul 17, 2022

Ok, turns out that my suggestion of simply catching the ValueError avoids the secondary crash, so I've just pushed that change to your PR. Hopefully should be good to merge now.

Thanks for reporting and tracking down the bug!

@ewels ewels enabled auto-merge July 17, 2022 18:51
@ewels ewels merged commit 7a6787b into MultiQC:master Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants