Release 6.3.0 - #84
Merged
Merged
Conversation
The CI Release Pipeline section claimed wheels for Linux x86_64 + aarch64, macOS x86_64 + arm64, and Windows. release.yml has only `linux` and `sdist` jobs, and 6.2.0 published exactly one wheel -- gbcms-6.2.0-cp311-cp311-manylinux_2_34_x86_64.whl -- plus the sdist. This matters beyond tidiness: everyone not on cp311 manylinux x86_64 installs from the sdist and therefore needs a Rust toolchain. That is all macOS users (Intel and Apple Silicon) and every Python other than 3.11. The old text would have you tell them `pip install gbcms` just works. Broadening the matrix is a change to release.yml, not a docs fix. Until someone makes that call, this section should say what actually ships. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uses The only way to change a setting was to build a whole GbcmsDnaConfig, whose four required fields -- variant_file, bam_files, reference_fasta, output -- have ZERO overlap with the seven this entry point reads. Two of them must name files that exist on disk, bam_files is a dict, and output is an OutputConfig. So adjusting one read filter meant fabricating an output directory and a variant path that are never touched: paths that read as load-bearing and are not, which is how the next person gets misled. Adds filters/quality/alignment/umi_tag/threads/apply_baq/library_type as keyword arguments. config= keeps working; an individual argument overrides the matching config field, so a pipeline config can be reused and adjusted. Found by the first real consumer (mulligan) needing a non-default read filter. Tests written inside gbcms could not have found it -- they never had to construct the config a caller does. umi_tag is the one argument where None is a choice rather than an absence: it means group by read pair, not UMI family. It uses a sentinel so that passing umi_tag=None alongside a config that sets one OVERRIDES it. Treating None as "not supplied" there would silently inherit the config's tag and change what counts as a molecule, with nothing to indicate it. Also corrects advice this module previously gave. The filters docstring told callers to set supplementary=False for cross-locus phasing. That does nothing: supplementary and secondary alignments are dropped before fragment evidence is built, so they never produce an observation. Measured across all six read filters on real data -- duplicates, improper_pair and indel work as documented; those two cannot change the result at all (#82). The new tests use `duplicates` as the discriminating filter for exactly that reason: `supplementary` would have been the intuitive choice and would have made the test pass vacuously. Tests: 420 passed (+11). ruff, black, mypy, clippy -D warnings, cargo test (215) all clean. Docs build warnings unchanged (1, pre-existing). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n turned off Closes #82. Both flags previously could not change ANY output. An unconditional skip in the counting loop discarded those records before fragment evidence, "regardless of the user filter flags" -- redundant when the flags were on (such records never reach the cache) and defeating them when off. Measured across all six read filters on real data, these were the only two that did nothing; duplicates, improper_pair and indel all worked. Default runs are byte-identical. Both still default to on, so the records never reach the cache: total depth unchanged at 1202 across 40 real loci, and binned<->legacy parity holds. Turning a filter off now admits those records to FRAGMENT-level evidence (dpf/rdf/adf) and to the observation export, while read-level dp/rd/ad keeps its promise and excludes them. That split is not a compromise, it is the correct line, and the existing tests say so. A supplementary shares a QNAME with its primary and is the same physical read, so counting both toward dp reports depth 2 where one read exists -- test_supplementary_shared_qname_not_double_counted pins exactly that case, and two more tests assert the same read-level contract. Honoring the flag naively broke all three. Fragments are immune either way: hash_molecule keys on QNAME, so a primary and its supplementary collapse into one FragmentEvidence, which is why admitting them there cannot double-count. What this fixes concretely: a locus reached ONLY by a supplementary segment reported dpf=0 -- not a filtered read but a wrong answer, since a molecule demonstrably covers it. That is what made a molecule spanning a large deletion invisible to cross-locus phasing. A new test covers that arrangement; it is the other half of the double-count test's contract. New consequence, documented in the read-filters page, the observations page and the CLI help: with the filter off an admitted supplementary raises dpf while leaving dp unchanged, so the two stop moving together. The observation reconciliation invariant (rows == dpf) is verified under both settings. Also documents the improper-pair foot-gun found while auditing: MSK-ACCESS v1 alignments are 100% paired but 0% PROPER_PAIR, so enabling that filter there discards every read. Tests: 421 passed (+1). clippy -D warnings, cargo test (215), mypy, ruff, black, mkdocs --strict all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat(observations): configurable observe_molecules; fix the secondary/supplementary filters (#82)
Bumps all 11 version references and dates the CHANGELOG entry.
Contents:
- observe_molecules() takes the settings it actually uses, instead of
requiring a GbcmsDnaConfig whose four required fields it never reads.
- --filter-secondary / --filter-supplementary now change the result when
turned off. They previously could not change any output. Default runs are
byte-identical; opting out admits those records to fragment-level evidence
and the observation export, while read-level dp/rd/ad still excludes them.
Minor rather than patch: new public arguments, and a filter that did nothing
now does something. Default behavior is unchanged either way -- verified on
real data (total depth 1202 across 40 loci, unmoved) and by the parity suite.
Gate: 421 python tests, 215 rust tests, clippy -D warnings, mypy, ruff, black,
mkdocs --strict all clean. Runtime version confirmed 6.3.0 after a release
rebuild. No stale 6.2.0 references outside historical prose.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Release PR for 6.3.0. Merge to
main, then tag6.3.0— bare, novprefix.What's in it
✨
observe_molecules()takes the settings it actually usesIt reads seven settings;
GbcmsDnaConfigrequires four, and they do not overlap at all. Worse,variant_fileandreference_fastamust name files that exist on disk,bam_filesis adict, andoutputis anOutputConfig— so changing one read filter meant fabricating paths that read as load-bearing and are not.New keyword arguments
filters,quality,alignment,umi_tag,threads,apply_baq,library_type.config=keeps working; an individual argument overrides the matching field.umi_taguses a sentinel becauseNoneis a real choice there — "group by read pair, not UMI family" — so passing it overridesconfigrather than being read as an omission. Without that, a caller could silently change what counts as one molecule.🐛
--filter-secondary/--filter-supplementarynow do somethingBoth previously could not change any output: an unconditional skip discarded those records before fragment evidence, regardless of the flags. Audited across all six read filters on real MSK-ACCESS data — these were the only two that were inert (
duplicates,improper_pair,indelall worked).The fix draws the line at read-level vs fragment-level, not on/off:
dp/rd/addpf/rdf/adfThat split is what the existing tests demand:
test_supplementary_shared_qname_not_double_countedbuilds a primary and its supplementary over the same locus and assertsdp == 1. Fragments are immune either way —hash_moleculekeys on QNAME, so the two collapse into one.Fixes concretely: a locus reached only by a supplementary segment reported
dpf=0— not a filtered read but a wrong answer. That is what made a molecule spanning a large deletion invisible to cross-locus phasing.Compatibility
Default runs are byte-identical. Both filters still default to on. Verified on real data: total depth unchanged at 1202 across 40 loci, and binned↔legacy parity green (18 tests).
New behavior to be aware of when opting out: an admitted supplementary raises
dpfwhile leavingdpunchanged, so the two stop moving together. Documented in the read-filters page, the observations page, and the CLI help.Minor rather than patch: new public arguments, and a filter that did nothing now does something.
Gate
pytestcargo testcargo clippy --all-targets -D warningsmypy/ruff check ./black --checkmkdocs build --strictdevelop(pre-existing)6.3.06.2.0references🤖 Generated with Claude Code