Refactor SAV module to follow MultiQC best practices #11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Apply improvements based on MultiQC module development guidelines: Critical fixes: - Add ModuleNoSamplesFound exception when no data found - Add add_data_source() and add_software_version() calls - Add write_data_file() call at end of module init - Export SAVModule class from __init__.py High priority fixes: - Change anchor from "SAV" to "sav" (lowercase) - Add href to Illumina InterOp GitHub repository - Improve info string (remove leading " - ") Medium priority improvements: - Add comprehensive class docstring documenting supported sequencers - Add scale attributes to all HEADERS entries for semantic coloring - Add general stats columns (% >= Q30, Yield, % PF, Error Rate) Low priority improvements: - Support multiple sequencing runs (removed single-run limitation) - Add defensive checks with .get() defaults in _clusters_lane_plot() - Update function type hints to use SAVModule type Also add multiqc_data* and multiqc_report* to .gitignore. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Member
Author
|
See example reports from the actions CI run: https://github.com/MultiQC/MultiQC_SAV/actions/runs/20332832779/artifacts/4909854845 Looks pretty good I think! I haven't taken a detailed look at the code changes, but they look sane. What do you think @matthdsm ? |
Collaborator
|
Looking good! I see some missing features that were previously there, but we can fix that in another PR. |
matthdsm
approved these changes
Dec 18, 2025
Collaborator
matthdsm
left a comment
matthdsm
left a comment
There was a problem hiding this comment.
Thanks for making quick work of this! ❤️
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
multiqc --strictpasses on MiSeq test datamultiqc --strictpasses on NovaSeq6000 test dataruff checkpassespre-commit run --all-filespassesDetailed changes
Critical fixes
ModuleNoSamplesFoundexception when no data foundadd_data_source()andadd_software_version()callswrite_data_file()call at end of module initSAVModuleclass from__init__.pyHigh priority fixes
"SAV"to"sav"(lowercase per convention)hrefto Illumina InterOp GitHub repository-)Medium priority improvements
scaleattributes to all HEADERS entries for semantic coloring% >= Q30,Yield,% PF,Error Rate)Low priority improvements
.get()defaults in_clusters_lane_plot()SAVModuletype🤖 Generated with Claude Code