Fixing MSI coverage handling and low-coverage classification - #13
Merged
Conversation
…implement sortable tables - Remove sample name duplicates by improving normalize_sample_name() regex - Replace standard green (#2ecc71) with light green (#d4edda) for better visibility - Convert summary table to custom HTML with sortable columns (click headers to sort) - Columns sortable by all metrics: percentage, low-coverage sites, etc. - Sort summary table by default: Percentage of unstable sites (descending), then low-coverage sites - Convert All Loci table to custom HTML to avoid MultiQC violin plot conversion warnings - Add light green color to All Loci Stable status cells - Add debug logging for sample name normalization - Maintain conditional formatting: Red for high unstable %, orange for low-coverage threshold - Generate JSON backup of all-loci data to prevent loss from MultiQC merging
- Change summary table sorting to prioritize 'Number of low-coverage sites' (descending) - Secondary sort by 'Percentage of unstable sites' (descending) - This ensures samples with high low-coverage counts appear first (e.g., D2510240 with 26 low-coverage sites now at position 1)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMSI Sensor Pro now parses and normalizes per-locus data, counts low-coverage sites, incorporates those counts into MSI classification, and renders expanded report tables. Configuration adds the low-coverage threshold and warning rule, and the package version is updated. ChangesMSI low-coverage reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SummaryFiles
participant AllLociFiles
participant MultiqcModule
SummaryFiles->>MultiqcModule: parse_summary()
AllLociFiles->>MultiqcModule: parse_all()
MultiqcModule->>MultiqcModule: annotate_summary_low_coverage()
MultiqcModule->>MultiqcModule: prepare_msisensorpro_data()
MultiqcModule->>MultiqcModule: assign Low-coverage, MSI-high, or MSS
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py`:
- Around line 94-136: Escape all dynamic report content before HTML generation
to prevent injection: update the table-building logic in the relevant rendering
function to apply html.escape() to sample names, cell values, column titles, and
any file-derived locus identifiers before inserting them into headings or data
cells. Import html as needed, while preserving numeric formatting and CSS
attributes; apply the same treatment to the additional affected block noted in
the comment.
- Around line 83-89: Remove unnecessary f-string prefixes from static HTML
fragments in the table-rendering code, including the styles and header markup
around the affected ranges, to resolve Ruff F541. In the exception handling for
value conversion in the same rendering logic, replace the bare or broad catch
with an explicit handler for the expected conversion exception, such as
ValueError (and TypeError only if applicable), resolving E722 without masking
unrelated errors.
- Around line 357-366: Update parse_all() so each locus is classified as
Low-coverage before evaluating pro_p against the instability threshold; ensure
low-coverage loci retain the “Low-coverage” status and are included by
annotate_summary_low_coverage(), rather than being labeled Unstable.
- Around line 143-158: Update sortTable’s numeric detection and comparison to
recognize percentage-formatted cell values such as “12.34%”: strip the trailing
percent sign and whitespace before parsing, then compare the resulting numbers
numerically while preserving existing string sorting for other values. Adjust
isNumeric and the numeric branch inside rows.sort accordingly.
- Around line 302-303: The low-coverage classification boundary in the relevant
classification logic is inconsistent with the report warning thresholds. Update
the condition assigning `msi_status` to use the same strict `>` comparisons as
the warning logic at Line 121 and the configuration, including the `num_sites`
check against `self.min_sites_threshold`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cd9da8c-fa07-40bc-96ec-10d76e3f23cb
📒 Files selected for processing (3)
docs/configs/multiqc_config_msisensorpro.yamlmultiqc_cmgg/__init__.pymultiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py
ToonRosseel
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py (3)
34-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid writing sample identifiers to logs.
These statements expose filename-derived sample identifiers at info/debug level. Log only aggregate counts and remove the raw-name traces.
Proposed fix
- log.info(f"Summary samples: {list(data_dicts_summary.keys())}") + log.info("Parsed %d summary samples", len(data_dicts_summary)) ... - log.info(f"All-loci samples: {list(data_dicts_all.keys())}") + log.info("Parsed %d all-loci samples", len(data_dicts_all)) ... - log.debug(f"parse_summary: raw_fn='{f['fn']}', raw_name='{raw_name}', s_name='{s_name}'") ... - log.debug(f"parse_all: raw_fn='{f['fn']}', raw_name='{raw_name}', s_name='{s_name}'")Also applies to: 370-370, 407-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py` around lines 34 - 36, Replace the sample-name lists logged after parse_summary and parse_all with aggregate counts only, such as the number of samples. Apply the same change to the corresponding logging statements at the other referenced locations, ensuring no filename-derived identifiers are emitted at any log level.
71-147: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the summary data export. The custom-HTML path only calls
self.add_section(content=summary_html), sodata_dicts_summaryis never written. Add an explicitself.write_data_file(data_dicts_summary, "msi_summary")here, like the all-loci section does, so downstream exports still exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py` around lines 71 - 147, The custom summary HTML path omits the summary data export. In the successful HTML-generation branch near summary_html and self.add_section, call self.write_data_file(data_dicts_summary, "msi_summary") before or alongside adding the section, matching the export behavior used by the all-loci section.
383-392: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve missing all-loci data as missing. When a sample has no matching
data_allentry,low_coverage_sitesbecomes0, which makes an unmatched sample look like a valid MSS/MSI-high call. Propagate a missing state or skip classification for unmatched samples instead of defaulting to zero.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py` around lines 383 - 392, Update annotate_summary_low_coverage so samples absent from data_all are not assigned low_coverage_sites = 0; detect missing sample entries and preserve a missing value or skip their classification. Keep the existing count for samples with matching data_all entries, and ensure downstream MSS/MSI-high classification recognizes unmatched samples as unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py`:
- Around line 287-303: Update write_all_loci_data_files to log failures from
both msiSensorPro_all_table and msiSensorPro_all_table_json exports at warning
level instead of debug, preserving exc_info=True; alternatively allow the
exceptions to propagate if these sidecar files are required downstream.
---
Outside diff comments:
In `@multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py`:
- Around line 34-36: Replace the sample-name lists logged after parse_summary
and parse_all with aggregate counts only, such as the number of samples. Apply
the same change to the corresponding logging statements at the other referenced
locations, ensuring no filename-derived identifiers are emitted at any log
level.
- Around line 71-147: The custom summary HTML path omits the summary data
export. In the successful HTML-generation branch near summary_html and
self.add_section, call self.write_data_file(data_dicts_summary, "msi_summary")
before or alongside adding the section, matching the export behavior used by the
all-loci section.
- Around line 383-392: Update annotate_summary_low_coverage so samples absent
from data_all are not assigned low_coverage_sites = 0; detect missing sample
entries and preserve a missing value or skip their classification. Keep the
existing count for samples with matching data_all entries, and ensure downstream
MSS/MSI-high classification recognizes unmatched samples as unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d668fc6-84cb-4ff8-9005-d696c96883c0
📒 Files selected for processing (1)
multiqc_cmgg/modules/msi_sensor_pro/msi_sensor_pro.py
matthdsm
reviewed
Jul 10, 2026
matthdsm
reviewed
Jul 10, 2026
matthdsm
reviewed
Jul 10, 2026
matthdsm
approved these changes
Jul 10, 2026
matthdsm
left a comment
matthdsm
left a comment
Contributor
There was a problem hiding this comment.
much better, much cleaner
matthdsm
reviewed
Jul 10, 2026
Co-authored-by: Matthias De Smet <11850640+matthdsm@users.noreply.github.com>
…MedicalGeneticsGhent/MultiQC_CMGG into msi_fix_coverage_issue
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 by CodeRabbit
New Features
Bug Fixes