Mathews 2004 parameter set (corrected) + paramset plumbing + dangles=2 exterior stacks - #13
Mathews 2004 parameter set (corrected) + paramset plumbing + dangles=2 exterior stacks#13KowalskiBio wants to merge 9 commits into
Conversation
EmilioVenegas
left a comment
There was a problem hiding this comment.
Holding off on approval until a few things are settled, but the parameter-set work looks solid. The three convention bugs you caught (stack key column orientation, the bulge_size leading zero that made 1-nt bulges free, and the dH interior tables that were computed but never emitted) all affected real results, and the point that the old straight-stem-only verification passed with the bulge bug present is a good case for the expanded set. Folding hairpin_thermo with the same paramset it scores with is a genuine fix. The provenance note follows the precedent already set for rna_turner2004.par.
Six things before this lands:
-
Tests. Neither code commit adds one: a 12k-line parameter set, a new
danglesoption on four entry points,paramsetthreading through six public functions, and a behavior change infraction_folded. The 658 passing tests are the pre-existing suite, andverify_against_viennaneeds ViennaRNA at dev time so it cannot run in CI. Enough to hold it in place would be a few pinnedmathews2004-dnaenergies, the "42/42 dangles=0 bit-identical" claim as a regression test, the paramset-by-name path that used to raise'str' object has no attribute 'dG', and afraction_foldedzero case. -
danglesdoes not reach the partition function. It is threaded into_mfe_nativeandsuboptonly, soThermoEngine(dangles=2)computes its MFE and its ensemble under different models, which affects ensemble defect, the differentiable path and concentration solving. The cache key gainedd{self.dangles}for every op including pfunc, which implies an effect that is not there. Either thread it through the McCaskill DP, or scope it explicitly to MFE and drop it from the pfunc key. Your call which, the second is much cheaper. -
The
dangles=2name. Best single negative dangle per exterior stem is not VR d2, which adds both flanks unconditionally, and your own numbers show it: two-flank cases drift 0.2 to 0.9, 38/42 within 0.4 kcal/mol. The docstring says "like VRmd.dangles=2" and the ValueError says "2 (VR dangles=2)". Since strider is judged on agreement with IDT and ViennaRNA, I would rename it or note the divergence at the API surface. -
The dimer path has no dangles.
hairpin_thermo(paramset=..., dangles=2)works,dimer_thermo(paramset=..., dangles=2)raises TypeError, and_sum_dimer_elementsnever sees the flag, though the motivating example in the PR body is a self-dimer. -
fraction_foldedcatchesValueErrortoo broadly. Thetrywraps the wholehairpin_thermocall, so a mistypedsalt_modelor badmaterialreturns 0.0 instead of raising. Only the "does not fold into a hairpin" case should map to zero. -
Citation. THIRD_PARTY_NOTICES cites Mathews et al. 1999, J Mol Biol 288:911-940 for a file named
dna_mathews2004.parfeedingmathews2004-dna.json. ViennaRNA ships both a 1999 and a 2004 DNA set, so worth confirming which this is before the preprint cites it.
Two smaller ones: _resolve_name is lru_cached and returns the same ParameterSet instance to every caller, so worth confirming nothing mutates one in place given the temperature-adjusted paramset machinery. And ext_e's 5' branch has a redundant (ko - 1) not in nicks check that drops a valid dangle when the flanking base starts a strand, where the 3' branch tests only jo + 1.
hairpin_thermo with a paramset and fraction_folded both change existing behavior, so they want changelog entries. #11 is merged, so this will need a rebase once #12 lands.
Lib/strider._native gains dimer_mfe_candidates, a statement-by-statement Rust port of strider.thermo.dimer_thermo._dimer_mfe_candidates (DNA): the inter-strand DP fill, dangles, terminal penalties, and the full Mathews-Turner interior-energy set (exact 1x1/1x2/2x2 tables included). Sequence-independent table data is NOT hand-copied: native/codegen_tables.py packs strider.thermo.parameters_dna keys (2-bit-per-base) and emits sorted static arrays (native/src/tables_dna.rs, 16 tables / 12694 entries) that the Rust side binary-searches. strider.thermo.dimer_thermo prefers the native DP when material='dna' and no param override is active (param_context or engine custom ParameterSet); all other cases keep the pure-Python implementation, which also stays as the test oracle (_dimer_mfe_candidates_py). Parity (tests/test_native_dp_parity.py): * candidate-list fuzz: 600 random pairs, 0/600 energy or pair mismatches * end-to-end: dimer_thermo/dimer_thermo_subopt (incl. ValueError behavior) 300/300 identical with the DP toggled between native and Python Timing on the Oligool IDT-analysis workload (2 hairpins + 3 dimer pairs): 41.5 ms -> 32.1 ms wall; the dimer DP segment (32 ms cumulative) drops to ~2 ms. Full suite unchanged: 649 passed, 4 pre-existing ViennaRNA-env failures, no regressions.
…ram-override guard test Addresses review feedback on the DP port: Alphabet divergence (EmilioVenegas#1): * tables.rs: CODE_TABLE defaults unknown bytes to u32::MAX (not 3=T), so any degenerate base (N/R/Y/...) produces a packed code that never matches a generated entry — binary-search misses, lookup returns None, the caller uses the Python default, exactly like dict.get(key, default) * pack(): early-return on sentinel so the check costs one branch per key * dimer_thermo.py: alphabet guard in the native wrapper rejects non-ACGT sequences and falls back to Python — the primary gate * tests: degenerate-base sequences now in the fuzz corpus (15%); explicit N/R cases in test_fallback_guard_degenerate_alphabet; test_native_raw_degenerate_matches_python proves the sentinel fix works even for direct (wrapper-bypassing) raw calls Codegen staleness (EmilioVenegas#2): * codegen_tables.py docstring now correctly references the test that validates the committed tables (it previously claimed a check that did not exist) * test_tables_dna_regenerated_matches_committed: regenerates from parameters_dna and compares byte-for-byte; catches silent drift when the Python tables are edited but the Rust ones were not regenerated Param-override guard (EmilioVenegas#3): * test_fallback_guard_param_override: _param_override with an empty dG dict forces the Python path (mirroring lookup_table fallback) * test_fallback_guard_custom_engine_params: engine with _uses_custom_params() = True forces the Python path 8/8 new + existing postgres tests pass; full suite unchanged (654 passed, 4 pre-existing ViennaRNA-environment failures).
The native-parity job only ran tests/test_native_parity.py; the DP fuzz, the guard tests and the codegen staleness check live in tests/test_native_dp_parity.py, which never executed in CI. Run both.
…s + dH interior tables - convert_stack: the .par matrix's column pair is written in reversed orientation (from the j-side); keys must read outer[0] + inner[1] + inner[0] + outer[1]. Verified against RNA.eval_structure_verbose: GCGC/CGCG = -2.2, CCGG/GGCC = -1.8, AATT/TTAA = -1.0, ATAT = -0.9, TATA = -0.6. - convert_bulge_size: drop the leading 0.0 'sentinel' — index 0 IS the 1-nt bulge entry (2.9). The bug made single-base bulges free and shifted every larger bulge by one size — the root cause of bogus single-bulge hairpin structures the DP over-stabilized. - convert_interior_size: same off-by-one fix (index i = VR internal[i+1]); size-table sentinels are now 30.0 (INF), matching the native set. - generate_paramset: emit the previously computed-but-dropped interior_1_1 / interior_2_2 dH tables, so structure_enthalpy no longer silently mixes native dG values into the enthalpy walk. - verification: add bulge-containing hairpins to the test set — the old straight-stem-only set passed with the bug present. 7/7 structures now match ViennaRNA eval_structure exactly at dangles=0.
…2 exterior stacks
- dimer_thermo / dimer_thermo_subopt / dimer_tm accept paramset (instance
or name) and use it to predict AND score the inter-strand helix,
matching the hairpin API.
- param_context + structure_* walkers resolve parameter-set NAMES via a
memoized loader instead of crashing on str ('str' has no .dG).
- hairpin_thermo folds with the same paramset it scores with
(param_context wrapping fold_mfe); fraction_folded returns 0.0 for
non-folding sequences instead of raising ValueError.
- NEW: dangles option (default 0, bit-identical energies) on fold_mfe /
subopt_structures / fold_complex / ThermoEngine(dangles=...) — exterior
dangling-end stacks at W-matrix stem placements: best negative single
dangle per exterior stem. Verified against ViennaRNA eval on a 25-case
single-stem flank grid: single-flank cases match exactly, two-flank
cases track the better of the two flanks; 38/42 end-to-end sequences
within 0.4 kcal/mol of VR(dangles=2) (remaining are G.T-wobble stems).
- hairpin dG/dH structure walk applies the same exterior-tail dangle, so
hairpin_thermo stays energy-consistent with the dangle-aware MFE.
…2004 paramset scripts/generate_mathews2004_params.py uses ViennaRNA's distributed dna_mathews2004.par as a development-time extraction input to produce strider/thermo/parameters/mathews2004-dna.json; the transcribed values are primary literals of Mathews et al. 1999 (JMB 288:911-940) — physical measurements, not copyrightable expression — and the JSON is self-contained static data. Neither the .par file nor ViennaRNA is redistributed or required at runtime.
…er flag, tests, notices * fraction_folded: zero case derives from MFE pairs; caller mistakes raise again (bad salt model, degenerate entropy, multiloop MFE) instead of being flattened to 0.0 by the blanket ValueError catch. * dangles scope: engine trim to MFE/suboptimal - pfunc/ensemble/defect/ differentiable/equilibrium explicitly ignore the flag; dropped from the pfunc cache key. Naming note at the API surface for the dangles=2 vs VR dangles=2 divergence (best single negative dangle per exterior stem). * dimer path: dimer_thermo/_subopt/_tm accept dangles=0|2 threaded into _sum_dimer_elements (each terminus gathers adjacent negative dangle stacks). Also a latent-domain fix: the enthalpy walk no longer ingests DG-valued dangle tables (dangle tables are DG-only). * ext_e 5' branch: drop the redundant (ko - 1) not in nicks check that discarded valid strand-start 5' dangles in fold_mfe/fold_complex. * notices: value-level confirmation of the dna_mathews2004.par identity (ATAT -0.9 vs -0.8, GT/CG +1.2 vs +1.3 markers vs the 1999 par file). * _resolve_name: documented the shared-instance read-only contract (no in-place writes exist in the tree today). * tests/test_mathews2004.py (12 tests): pinned mathews2004-dna folded energies and table markers, 42-case dangles=0 bit-identity grid, name-path regression, fraction_folded zero + error propagation, dimer dangles flag, pfunc/cache-key scoping, strand-start flank regression pin. * changelog: behavior-changes and documentation entries for the above.
3586df7 to
0b0145a
Compare
|
All six items, the two smaller ones, and the changelog are in at 1. Tests — new
2. dangles scoping — took the cheap option: 3. dangles=2 naming — kept the name but placed the divergence note at the API surface: the 4. Dimer path — 5. 6. Citation — re-confirmed at value level: the JSON matches Smaller ones:
Changelog — three entries under Verification — full non-slow suite on the rebased tree: 669 passed, same 4 pre-existing |
ViennaRNA marks 8 hairpin-mismatch dG entries as DEF but resolves them to -0.50 kcal/mol at runtime. strider was treating DEF as 0.0, causing dG mismatches of 0.50 kcal/mol on any hairpin with those mismatch combinations (verified against eval_structure on 10k oligos). The dH values for these entries ARE defined in dna_mathews2004.par but were also discarded. Now always stored when defined (96 entries, up from 73). Hardcode the 8 known DEF dG entries to -0.50 via a lookup set, since the .par file does not encode the default and ViennaRNA resolves it internally. All other DEF entries remain 0.0 (the correct default for non-WC pair types). Regenerated mathews2004-dna.json passes all verification checks against ViennaRNA eval_structure at dangles=0.
|
Thanks, the review round is thorough and I am happy with all six. Scoping #11 and #12 are both merged now, so this wants a rebase onto main. On the 10k benchmarkThe hairpin ΔG panel is the strongest result in it. Three things about the analysis before we lean on the rest of it: Compare on the common subset. With Mathews at 26.5% NA and SantaLucia at 35.9%, and the plots filtered to finite values, the two methods are being scored on different sequence populations, and the harder cases drop out preferentially. Could you rerun restricted to oligos where all four methods return a value, with the NA rate reported separately as its own metric? As it stands, a change that turns hard cases into NAs reads as an accuracy gain. The duplex Tm slope is probably not ours. Every method including primer3 drifts from about -2 at IDT Tm 50 to +5 at Tm 78, in near-parallel bands. Four independent implementations do not usually share a slope, so I would treat that as an IDT-side convention and chase only the per-method offsets (strider_m +1.3, strider_sl -2.3, primer3 near 0). Worth ruling out condition mismatch first: Na, Mg, dNTP, oligo concentration and the reporting temperature on both sides. The hairpin Tm outliers are shared. strider_m reaches +250, but primer3 hits +80 and Vienna +120 on the same set. A failure mode common to all four points at the comparison rather than at strider, most likely oligos where a two-state Tm is not defined. I would pull those specific sequences and look at them before keeping a 200 °C ceiling, since the ceiling hides the cases instead of explaining them. Also worth noting that homodimer ΔG is where strider is weakest, both variants around +2.2 to +2.6 while primer3 and Vienna sit near zero, and Mathews does not fix it. That is unaffected by this PR except that the dimer dangle default change and the ΔH domain fix will both move it, so the panel needs regenerating afterwards either way. Request: split the hairpin commits out
The reason I want them separate: the Vallone hairpin set is measured Tm, and it is what established that strider is near-unbiased and that the beacon offset is probe-specific. Everything in the 10k benchmark measures agreement with another prediction tool. Tuning defaults toward IDT can move us away from the experimental agreement the validation rests on, and neither the repo suite nor this benchmark would show it. So: keep the parameter set, the plumbing, Two smaller notes on that code for when it moves. The candidate loop in the multiloop path uses One question rather than a request: is the 25 °C fold chosen because IDT reports hairpin analysis at 25 °C? If so that is a much better justification than the NA rate, and it argues for exposing it as a named parameter with a documented default so the 37 °C behavior stays reachable. |
ensemble_dg and multistrand_pairs now accept pair_probs=True (default, preserves existing behavior). When False, skip _external_Q_nodangle and _pair_probs_outside and return a zero matrix; dG is byte-identical because the inside partition (_fill_dp_nicks + _apply_coaxial_external) fully determines Q[0][n-1] before the outside pass runs, and the outside pass writes only to local arrays. Thread the flag through ThermoEngine.pfunc -> _pfunc_dispatch -> _pfunc_native -> _pfunc_native_inner. Use a distinct 'pfunc_dg' cache-key op for the no-probs variant so cached results never cross-contaminate, and extend the dangles-exclusion check to cover it.
924f46c to
cea7364
Compare
|
Split done. The two hairpin commits ( What stays in #13: parameter set, plumbing, dangles, DEF fix, and the What moved to #14: 25 C fold, multiloop splitting, degenerate-dS/Tm guards, plus the two review notes fixed:
On the 25 C question: yes, 25 C is because IDT reports hairpin analysis at 25 C. On the Vallone set: it is not in the repo. The 10k parity table is prediction-vs-prediction and cannot substitute. #14 notes this explicitly and recommends running the Vallone set under old and new behavior before it lands. |
Summary
Stacked behind #11 (
native-tm) and #12 (native-dp) — review/merge those first: this branch builds onnative-dp, so until #12 merges this PR's diff also shows #11's and #12's commits. After #12 merges, the effective diff here is exactly the 2 commits at the tip.This PR adds a complete, corrected Mathews 2004 DNA parameter set (
strider/thermo/parameters/mathews2004-dna.json) with the plumbing to use it at every API surface, plus a dangles=2 exterior dangling-end option on the MFE DP. All opt-in: with noparameter_setanddangles=0(both defaults), behavior is bit-identical to v1.2.x.What's in here
1.
strider/thermo/parameters/mathews2004-dna.json+scripts/generate_mathews2004_params.pyThe JSON is generated from ViennaRNA's
dna_mathews2004.parat development time (docstring in the generator covers the license argument: numeric thermodynamic values are published scientific data; the generator is a dev tool, no runtime dependency). During review-by-comparison against ViennaRNA's own energy evaluation, three convention bugs were caught in my first draft of the generator — all fixed here, each verified againstRNA.eval_structure_verbose:outer[0] + inner[1] + inner[0] + outer[1]. Verified:GCGC/CGCG= −2.2,CCGG/GGCC= −1.8,AATT/TTAA= −1.0,ATAT= −0.9,TATA= −0.6.bulge_sizeoff-by-one: a leading0.0"sentinel" made index 0 (the 1-nt bulge entry) free instead of 2.9 — the root cause of DP-absurd single-bulge hairpin structures. Every larger bulge was also shifted by one size.interior_sizeoff-by-one + INF sentinels (same alignment class as bulge).dHinterior_1_1 / interior_2_2 tables: they were computed by the converter but never emitted, sostructure_enthalpysilently fell back to native ΔG values inside the ΔH walk.Verification (built into the generator,
verify_against_viennaunderpython scripts/generate_mathews2004_params.py): 7/7 test structures match ViennaRNAeval_structureto 0.00 kcal/mol at dangles=0, including three bulge-containing hairpins that would have caught the bulge misalignment (the previous straight-stem-only set passed with the bug present).2. Paramset plumbing
dimer_thermo/dimer_thermo_subopt/dimer_tmacceptparamset(instance or name) and use it to predict and score the inter-strand helix, symmetric with the hairpin API.hairpin_thermofolds with the same paramset it scores with (previously it folded native and then scored custom — two different models).param_contextand the fourstructure_*walkers resolve parameter-set names via a memoized loader (load_parametersstring input previously crashed with'str' object has no attribute 'dG').hairpin_tm,fraction_foldedacceptparamset/dangles;fraction_foldedreturns0.0for non-folding sequences instead of raisingValueError(a non-folding beacon's melt is a flat zero curve).3.
danglesoption (default 0 → bit-identical)Exterior dangling-end stacking at W-matrix stem placements in the linear-kernel DP (
fold_mfe,subopt_structures,fold_complex,ThermoEngine(dangles=...)). Rule: best negative single dangle per exterior stem end (equivalent to the d1/d2 outer-loop behavior ViennaRNA reports for single-stem structures).Empirically verified against ViennaRNA:
_sum_elements) applies the identical exterior-tail dangle, sohairpin_thermostays energy-consistent with the dangle-aware MFE;dangles=0keeps every existing energy bit-identical (validated: fold↔walk consistency 13/13).Why
Matching IDT OligoAnalyzer's published numbers for oligo secondary structure (their analysis family encodes Mathews-2004 DNA NN + dangle stacking at helix termini at 25 °C). Two worked examples from the app where this moves strider from "obviously wrong" to "within noise of IDT":
Test plan
TestNativeViennaParitydimer envelope vs local ViennaRNA — verified identical on the pristine v1.2.x baseline); 10 skipped, 1 xfailed (unchanged baseline).dangles=0regression: energies bit-identical across the test battery (42/42 fold↔walk consistent).