Skip to content

Filter vcf bug fixes - #68

Merged
alex-sandercock merged 4 commits into
SAT-151-allow-iupac-codes-on-match-allelesfrom
filterVCF_updates
Aug 5, 2026
Merged

Filter vcf bug fixes#68
alex-sandercock merged 4 commits into
SAT-151-allow-iupac-codes-on-match-allelesfrom
filterVCF_updates

Conversation

@alex-sandercock

Copy link
Copy Markdown
Collaborator

This pull request makes several important improvements and bug fixes to the filterVCF() function and its documentation, focusing on more robust handling of VCF INFO field parsing, clearer and more accurate documentation, and improved filtering logic. The changes ensure that filtering is consistent, transparent, and less error-prone, especially when dealing with scientific notation and missing values.

Bug Fixes and Filtering Logic Improvements:

  • Fixed parsing of INFO field values written in scientific notation (e.g., PMC=4.78e-07) by using as.numeric() on the entire field, ensuring correct filtering of variants and preventing the loss of high-quality SNPs. [1] [2]
  • Variants with unreadable or missing filter values (e.g., missing OD, BIAS, PMC, or MAF) are now explicitly removed and the number removed is reported in a warning, preventing the creation of corrupt all-NA variants and ensuring users are aware of data loss. [1] [2] [3]
  • All requested filters are now applied to every record, regardless of whether the INFO field is present in the first record, and records missing a usable value are removed and reported. This removes order-dependence and silent filter skipping. [1] [2]

Documentation and Usability Improvements:

  • The documentation for filterVCF() (in both Roxygen and Rd formats) has been corrected and expanded to accurately describe each filter's behavior, clarify the filtering order, and specify what is returned by the function. [1] [2] [3] [4]
  • The order and interaction of filters are now clearly documented, including the distinction between per-genotype and per-variant filters, and the exact behavior when outputting files or vcfR objects. [1] [2]

These changes make the filtering process more reliable, transparent, and user-friendly, reducing the risk of silent data loss or unexpected results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR improves filterVCF()’s correctness and transparency by fixing INFO-field parsing (notably scientific notation), making INFO-based filters consistently applied across all records, and ensuring variants with unreadable/missing filter values are removed (with explicit warnings) rather than producing corrupt all-NA rows.

Changes:

  • Reworked INFO field extraction to correctly parse full numeric representations (including scientific notation) and to match full INFO entries (avoiding substring collisions).
  • Added explicit removal + warnings for variants with missing/unreadable values needed by requested filters (including MAF).
  • Updated documentation (Roxygen + Rd), NEWS entry, and expanded test coverage to lock in the new behaviors.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
R/filterVCF.R Fix INFO parsing + apply INFO filters consistently; remove variants with unusable filter values and warn.
tests/testthat/test-filterVCF.R Adds/updates tests covering scientific notation parsing and removal of unreadable/missing filter values.
man/filterVCF.Rd Regenerated Rd documenting filter semantics/order and return behavior.
NEWS.md Changelog entries describing the filtering/parsing fixes and doc updates.
Files not reviewed (1)
  • man/filterVCF.Rd: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/filterVCF.R

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • man/filterVCF.Rd: Generated file
Suppressed comments (1)

R/filterVCF.R:211

  • select_variants() only treats NA values in values as unusable. If the filter comparison itself produces NA (e.g., because a user supplies filter.OD = NA or another threshold coerces to NA_real_), keep will contain NAs and vcf[keep, ] can again insert corrupt all-NA variants—the behavior this PR is trying to eliminate. Ensure keep never contains NA before subsetting (and ideally warn when comparisons evaluate to NA).
  select_variants <- function(keep, values, label, hint) {
    unusable <- is.na(values)
    if (length(values) > 0 && all(unusable)) {
      # Losing every variant is usually a problem with the input rather than a
      # genuine result, so this case says what to look at instead of only
      # reporting the count.
      warning("No readable ", label, " values were found, so all ", length(values),
              " variants were removed. ", hint, call. = FALSE)
    } else if (any(unusable)) {
      warning(sum(unusable), " of ", length(values), " variants had a missing or ",
              "unreadable ", label, " value and were removed.", call. = FALSE)
    }
    return(keep & !unusable)
  }

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.34%. Comparing base (a851de7) to head (cbcfb19).

Additional details and impacted files
@@                              Coverage Diff                               @@
##           SAT-151-allow-iupac-codes-on-match-alleles      #68      +/-   ##
==============================================================================
+ Coverage                                       80.92%   81.34%   +0.41%     
==============================================================================
  Files                                              19       19              
  Lines                                            2349     2385      +36     
==============================================================================
+ Hits                                             1901     1940      +39     
+ Misses                                            448      445       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alex-sandercock
alex-sandercock merged commit d4f0029 into development Aug 5, 2026
8 checks passed
@alex-sandercock
alex-sandercock deleted the filterVCF_updates branch August 5, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants