Skip to content

fix(pysam): retain variant summaries with non-finite QUAL values - #316

Open
ketpatil77 wants to merge 1 commit into
K-Dense-AI:mainfrom
ketpatil77:fix/validated-input-edge-cases
Open

ketpatil77 wants to merge 1 commit into
K-Dense-AI:mainfrom
ketpatil77:fix/validated-input-edge-cases

Conversation

@ketpatil77

@ketpatil77 ketpatil77 commented Oct 15, 2026 •

Copy link
Copy Markdown

A local VCF containing a nan QUAL value caused variant_summary.py to exit 2 when strict JSON serialization encountered a non-finite mean. HTSlib also accepts positive and negative infinity.

Exclude non-finite qualities from the mean and count them separately as record_counts.nonfinite_quality, while preserving all variants in other counts. Document that behavior and bump the pysam skill metadata version.

Validation: reproduced the CLI failure before the fix; all three non-finite regressions failed before the change. python -m pytest tests/pysam -q: 63 passed, 33 subtests passed. Repository meta checks: 18 passed, 4,224 subtests passed. No remote genomic data was accessed.

@rahul05ranjan rahul05ranjan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified the crash path and the fix mechanics locally (my host doesn't have pysam, so I couldn't run tests/pysam itself — details below).

  • Confirmed the script serializes with allow_nan=False (variant_summary.py line 340), so on main a nan QUAL falls into the else branch and poisons the mean: json.dumps({"mean_quality": nan}, allow_nan=False) raises ValueError: Out of range float values are not JSON compliant. That matches the reported exit 2, and inf/-inf would hit the same wall.
  • Replicated the new branch logic (lines 246–254) with a stdlib snippet: [nan, 50.0] → nonfinite_quality=1, mean_quality=50.0, serializes clean; [inf, -inf, 50.0] → nonfinite_quality=2, mean still 50.0. The all-non-finite edge is also safe: mean_quality is already guarded by if quality_count (line 308) and becomes None, which passes strict JSON.
  • One observation: since record_counts is a Counter, nonfinite_quality only appears in the report when non-finite QUALs exist — but missing_quality behaves the same way, so it's consistent with the existing shape.

Nice minimal fix, and the docs paragraph plus the metadata bump cover the behavior change. LGTM.

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