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

New module: Porechop #1728

Merged
merged 23 commits into from Jan 1, 2023
Merged

New module: Porechop #1728

merged 23 commits into from Jan 1, 2023

Conversation

jfy133
Copy link
Contributor

@jfy133 jfy133 commented Jul 20, 2022

Adds module for Porechop

  • 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: Add test data for Porechop module test-data#235
  • 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

Example output:
multiqc_report.zip

@jfy133 jfy133 changed the title Adds module for Porechop New module: Porechop Jul 20, 2022
@jfy133
Copy link
Contributor Author

jfy133 commented Jul 21, 2022

I think tests are failing possibly because it's taking too long to search for the string inside each log file... need to work out how to speed up I guess . I guess theoretically via number of lines but I'm not sure if this is consistent between porechop runs, will need to compare

docs/modules/porechop.md Outdated Show resolved Hide resolved
@ewels
Copy link
Member

ewels commented Jan 1, 2023

I'm tempted to combine the 3 plots into 1, with 3 datasets. Feels quite verbose for fairly simple stats. But I'll leave it for now as it's late and I should be in bed.

It's an easy change if you agree though: https://multiqc.info/docs/#switching-datasets

@ewels ewels merged commit 907a9e5 into MultiQC:master Jan 1, 2023
@jfy133
Copy link
Contributor Author

jfy133 commented Jan 2, 2023

Thanks @ewels ! I wasn't sure what was the normal practise here, looking through other examples it looked like switching was between counts and percentages, and the percentages were calculated by the module itself and wasn't derived from the log files themselves.

Therefore in this case were the percentages were in the porechop log files, I thought it better to plot each set of raw data separately - but I have no preference either way.

@ewels
Copy link
Member

ewels commented Jan 2, 2023

I don't mean the percentages really - that bit is fine. I more meant combining the plots of the three different metrics (start, end, split) into one section / plot. Just with tabs to move between them. Each would still have counts / percentages in the same way.

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.

None yet

2 participants