Repository navigation
Support smallest and largest subunit mode for RLEs - #665
Open
theferrit32 wants to merge 10 commits into
Open
theferrit32 wants to merge 10 commits into
theferrit32 wants to merge 10 commits into
Conversation
For a reference-derived ambiguous insertion, more than one factor of the seed length may be circularly expandable to recreate the alternate sequence. VRS <= 2.0 selects the greatest such factor as the repeatSubunitLength; ga4gh/vrs#700 changes this to the smallest. Add RleSubunitMode.{LARGEST,SMALLEST} and thread it through _normalize_allele(), normalize() and the translator (instance default plus per-call kwarg, mirroring rle_seq_limit). _factor_gen() now yields factors ascending or descending per the mode, so the first valid factor found is the selected one. repeatSubunitLength is inherent to the ReferenceLengthExpression digest, so the modes can yield different computed identifiers for one input Allele. LARGEST remains the default; behavior is unchanged unless the mode is passed explicitly. Only step 6.c (reference-derived ambiguous insertion) is affected. Reference-agree alleles, substitutions and deletions take repeatSubunitLength from the seed length directly and are mode-invariant. Refs #637
Declarative table of 11 alleles asserted under both RleSubunitMode values, using the local seqrepo dataproxy so no cassettes are involved. Differing cases show LARGEST's repeatSubunitLength tracking the insertion size rather than the repeat unit: A x 9 -> A x 11 gives 2 vs 1, A x 9 -> A x 18 gives 9 vs 1, and a GT microsatellite insertion of 2 and 3 units gives 4 and 6 vs 2. A x 9 -> A x 20 covers the prime-seed fall-through where LARGEST already yields 1. Mode-invariant cases pin the branches the spec change does not touch: reference-agree, substitution, deletion, a short tandem duplication, and a CAG insertion whose seed length exceeds the modified reference length. Also assert the default mode still equals LARGEST, and that each RLE state denormalizes back to its literal sequence under either mode, so translate_to output is unaffected. Refs #637
Explicitly declare the 14 pre-existing corpus alleles that normalize to a ReferenceLengthExpression, along with the RLE parameters and the literal alternate sequence each reconstructs to, and assert the full round trip input -> RLE -> denormalize_reference_length_expression() -> alt. The table supplies the alt sequences the *_normalized dicts omit: most of those tests pass rle_seq_limit=0, so state.sequence is absent and only the reconstructed length could be checked. allele_dict2 is excluded because its indefinite Range positions give no single reference span to reconstruct from.
…agree - Collapse the five accession constants onto the four sequences they actually name. _HOMOPOLYMER_AC and _MICROSAT_AC were the same chr1 accession, and the comments on two others named the wrong chromosome (chrX was labelled NC_000002.12, chr6 was labelled NC_000011.10). - Rename homopolymer_* ids to poly_a_* and describe each locus at its case, so ids describe the behavior under test rather than the substrate. - Correct the tandem_dup_gt and trinucleotide_ins_cag comments. Both claimed the modes agreed because larger factors were too long; in fact the deciding factor is that every competing candidate is rejected as an invalid cycle. - Fix step numbers to match the 0-indexed spec list at vrs.ga4gh.org/en/stable/conventions/normalization.html: the factor search is 5.c.1, returning via 5.d; reference-agree is 2.a, substitution 2.b, deletion 5.b. Restores the numbering the corpus comments already used. - Add an `agreement` field and test_normalize_rle_subunit_mode_agreement, which observes the step 5.c.1 candidates to assert *why* the modes agree, so a case drifting between categories fails instead of silently still passing.
…ings Use one constant for all rle_subunit_mode defaults and describe the modes by VRS version.
korikuzma
reviewed
Oct 5, 2026
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class RleSubunitMode(str, Enum): |
Contributor
There was a problem hiding this comment.
In the 2.1 branch, we'll only allow smallest, right?
Contributor
Author
There was a problem hiding this comment.
@korikuzma my plan was to just change the default to smallest just in case someone wanted to generate a hash based on using the largest subunit. But we could remove support for largest entirely too. I have no strong opinion since it's not too many lines to keep around.
Contributor
There was a problem hiding this comment.
IMO I think we should only allow smallest in 2.1 branch so that people don't create invalid digests
korikuzma
reviewed
Oct 9, 2026
korikuzma
left a comment
korikuzma
left a comment
Contributor
There was a problem hiding this comment.
i realized i forgot to hit submit
| `repeatSubunitLength` for a reference-derived ambiguous insertion. Defaults to | ||
| `DEFAULT_RLE_SUBUNIT_MODE`. See `RleSubunitMode`. | ||
| """ | ||
| rle_subunit_mode = RleSubunitMode(rle_subunit_mode) |
Contributor
There was a problem hiding this comment.
I don't think this is necessary if we're requiring an enum member to be passed
Suggested change
| rle_subunit_mode = RleSubunitMode(rle_subunit_mode) |
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class RleSubunitMode(str, Enum): |
Contributor
There was a problem hiding this comment.
IMO I think we should only allow smallest in 2.1 branch so that people don't create invalid digests
This branch has not been deployed
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.
Closes #637.
Adds RleSubunitMode to choose which valid factor normalization picks as the repeatSubunitLength of a reference-derived ambiguous insertion:
The mode can be set through normalize(..., rle_subunit_mode=...), on the AlleleTranslator constructor, or per call to translate_from. Only reference-derived insertions are affected; other normalization paths ignore the mode.
The vrs/2.1 branch will make SMALLEST the default, in line with vrs spec v2.1.
vrs-annotate doesn't expose an option to switch between largest/smalleset, and only uses the default. This is intentional.