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: 10x Space Ranger #1945

Merged
merged 48 commits into from
Feb 28, 2024
Merged

New module: 10x Space Ranger #1945

merged 48 commits into from
Feb 28, 2024

Conversation

grst
Copy link
Contributor

@grst grst commented Jul 1, 2023

  • 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)
    • similar to cellranger there are many relevant metrics that are best represented as table
    • I tried to hide less common ones by default.
  • 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

Add a module for 10x Space Ranger.

Since the output reports are very similar (yet not identical) to the Cellranger reports, I started by copying the cellranger module.

@grst grst changed the title Add Space Ranger module New module: 10x Space Ranger Jul 11, 2023
@grst grst marked this pull request as ready for review July 11, 2023 08:23
@ewels
Copy link
Member

ewels commented Aug 3, 2023

For next time (I know you copied this from the CellRanger module so this isn't really anything to do with you @grst, but still I feel compelled to say) - please don't make MultiQC module code too clever 😅

I'm trying to tweak the General Stats header config and I'm spending ages going around in circles trying to find where the attributes are set - in which function, which file, which dict. It's really much better to have verbose repetitive code that is easy to read, standardised with other MultiQC modules, and trivial to customise on a per-column basis. Even though everyone's knee-jerk response to seeing repetitive code is to optimise into a function, everyone does it differently and it hinders customisation - it causes a lot of hassle.

Rant / shouting into the void over... 😬

@ewels
Copy link
Member

ewels commented Aug 3, 2023

Mostly looking good, going from the report - thanks for this!

Quick thoughts:

  • Can we chop the decimal off a bunch of the table values that are integers (gene counts, spot counts etc)
  • In the table with pass / fail status - what are the blanks? Are they passes? Can they be marked as such?
  • If we could check that the line charts data being downsampled for the plot, so that each line doesn't have 11k data points in the report and creates a massive filesize. See config options smooth_points (docs)

@grst
Copy link
Contributor Author

grst commented Aug 15, 2023

Hi @ewels,

I agree with pretty much everything of your rant. I needed a lot of time myself to understand the logic of the Spaceranger module and was particularly unhappy with the way Mixture classes are used here -- I still figured it was the fastest way to get the job done to reuse the code.

Regarding your specific feedback:

Can we chop the decimal off a bunch of the table values that are integers (gene counts, spot counts etc)

should be fixed

In the table with pass / fail status - what are the blanks? Are they passes? Can they be marked as such?

the problem is that the cellranger/spaceranger HTML reports do not report passes, only failures. So yes, we could mark them as passes, but we never get the full matrix, only the samples that have at least one failure and only the failures that appear in at least one sample show up in the table. I'd be inclined to leave it as-is, but happy to change it if you prefer.

If we could check that the line charts data being downsampled for the plot

I checked the JSON we extract from the spaceranger report and don't think any further downsampling is necessary. It's about 20 data points per sample.

Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

Merged with main, along with some minor clean-up, and good to merge.

@vladsavelyev vladsavelyev added module: new and removed awaits-review Awaiting final review and merge. labels Feb 25, 2024
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.

Looking good. Couple of very minor suggestions, but that's all.

Many thanks for your work on this @grst!

multiqc/modules/spaceranger/count.py Outdated Show resolved Hide resolved
multiqc/modules/spaceranger/count.py Outdated Show resolved Hide resolved
docs/modules/spaceranger.md Outdated Show resolved Hide resolved
docs/modules/spaceranger.md Outdated Show resolved Hide resolved
vladsavelyev and others added 13 commits February 28, 2024 13:57
Co-authored-by: Phil Ewels <phil.ewels@seqera.io>
Co-authored-by: Phil Ewels <phil.ewels@seqera.io>
…po (MultiQC#2399)

* When checking for git SHA, make sure we are not inside another git repository

* [automated] Update CHANGELOG.md

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
* Violin plot export: use metric titles instead of IDs

* Add Export button for table. Do not prefix violin ID, instead prefix table.

* Violin save_data_file: use titles instead of metric ids as well

* [automated] Update CHANGELOG.md

* Fix

* Fix linting

* CSP

* Wrap table buttons in .col-xs-12 too to add some bottom padding

* Use Export as CSV wording

* Fix the edge case

* Fix the edge case - 2

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
* Add option to place bar plot legend on the bottom

* [automated] Update CHANGELOG.md

* Update docs

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
…n table cells (MultiQC#2395)

* Violin plot export: use metric titles instead of IDs

* Add Export button for table. Do not prefix violin ID, instead prefix table.

* Violin save_data_file: use titles instead of metric ids as well

* [automated] Update CHANGELOG.md

* Fix

* Avoid &nbsp; in tables

* Fix linting

* Unnecessary nowrap

* Use a small space span before suffix

* [automated] Update CHANGELOG.md

* Also replace &nbsp; with a space in case if modules set it

* changelog

* Fix linting

* CSP

* Update old class name in the old highcharts theme

* Fix CSP

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
Co-authored-by: Phil Ewels <phil.ewels@seqera.io>
* Move multiqc_data.json export out of megaqc.py and into util_functions

* Better error message

* [automated] Update CHANGELOG.md

* Keep script_path str

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
Co-authored-by: Vlad Savelyev <vladislav.sav@gmail.com>
@ewels ewels merged commit 35e9031 into MultiQC:main Feb 28, 2024
7 checks passed
@grst grst deleted the spaceranger-module branch February 28, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants