perf(cci,correl): test the divisor instead of reformulating around it (#395) - #398
Merged
Conversation
…#395) #397 fixed CCI and CORREL by dividing by the value the guard tests. That is the right shape, and it cost 11% and 14% on the shipped build. What it bought over simply testing the divisor is precision below ~1e-308 and ~1e-78 respectively -- magnitudes outside TA-Lib's operating domain, unreachable by the fuzz generator's own bar geometry, and quoted by no instrument. The regression is not worth it. Both now keep their original arithmetic and add an exact test on the divisor: CCI && 0.015*tempReal2 != 0.0 then tempReal/(0.015*tempReal2) CORREL && ssX*ssY > 0.0 then spXY/sqrt(ssX*ssY) Exact, not a band, in both: the flatness question is already answered by the tests above them, and these ask only whether the value the division uses exists. An absolute band on either would be the #253 defect at a new address. Post-guard the quotient is bounded -- by the period for CCI (|tempReal| <= n*tempReal2), by Cauchy-Schwarz for CORREL -- so a finite output needs no further argument. The defect #395 reported is still fixed. Such a window is now degenerate and takes the same 0.0 arm an identical-price window already did, which is what cci.yaml's silence about NaN/Inf and correl.md's "the output is 0 rather than an error or NaN" both already promised. What is given up is the correct CCI value in the band below ~1e-308 and correlations accurate past ~1e-78; both are answered 0. Value-neutral, which is the point and is measured three ways. LEGACY passes with CCI's row REMOVED, so it is bit-exact against frozen v0.6.4 again and returns to the list ta_test_legacy.c names; --fuzz-064's CCI count falls from 1365 tolerated cases back to 18, the pre-existing #7 ones; and an output hash over periods 2..60 of two corpora is bit-identical to the pre-#395 library for both functions. Perf, shipped-build A/B on a Zen 4 laptop, two full independent builds, min of 14 and 16 alternating rounds x 200/250 reps over 20k bars: CCI 0.999 and 0.980, CORREL 0.954 and 1.017, against a control spread of 0.953..1.011 across SMA, RSI, ATR and STOCH. Both sit inside the noise where they were 5x outside it. (An earlier attempt that linked two objects ahead of the archive rather than building two libraries moved RSI by 8% and was discarded.) The sweep clears CORREL without an annotation now: its clearing test looks through sqrt/fabs, which is the transparency the firing side already used, and is sound because sqrt(v) is zero exactly when v is. The inline arm's self-test is rebuilt around the shipped shape -- it removes guards rather than injecting a divisor -- and still pins all three halves: fires on a guard that names only an operand, clears when one names the divisor, silent when nothing bounds either. WILLR is untouched. Its change moves values on ordinary data, its batch tier came out ~8% faster, and the maintainer's clamp ruling stands. Green: full C suite, generator cargo test and clippy -D warnings, Rust clippy/doc/doctests/tests, --codegen against the frozen oracle (161 passed 0 failed in each of four backends), --xlang-hash (four backends bit-identical), --fuzz-064 (0 failures), regen-check, source-list/cargo-lock/stream-retcode checks. Both value legs re-sabotage-proven against a build of the pre-#395 arithmetic: CCI came back +/-Inf on all 39 bars, CORREL NaN/-1/+1. Claude-Session: https://claude.ai/code/session_01GesMuivQUwX4xHr4hEtnS8
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
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.
Follow-up to #397, on the maintainer's call: the reformulation that fix used cost 11% on CCI and 14% on CORREL, and what it bought over simply testing the divisor is precision below ~1e-308 and ~1e-78 — magnitudes outside TA-Lib's operating domain, unreachable by the fuzz generator's own bar geometry, quoted by no instrument. Not worth the regression.
Both functions keep their original arithmetic and add an exact test on the divisor:
Exact, not a band, in both — the flatness question is already answered by the tests above them, and these ask only whether the value the division uses exists. An absolute band on either would be the #253 defect at a new address.
The #395 defect is still fixed. Such a window is now degenerate and takes the same
0.0arm an identical-price window already did — which is whatcci.yaml's silence about NaN/Inf andcorrel.md's "the output is 0 rather than an error or NaN" both already promised. What is given up is the correct CCI value below ~1e-308 and correlations accurate past ~1e-78; both now answer 0.Value-neutral, measured three ways
ta_test_legacy.cnames.--fuzz-064CCI tolerated cases: 1365 → 18, the 18 being the pre-existing TA_CCI need a fix for when checking zero values #7 ones. 1347 cases returned to bit-exact.Performance
Two full independent builds, Zen 4 laptop, min of 14 and 16 alternating rounds × 200/250 reps over 20k bars:
Both sit inside the noise where they were 5× outside it. (An earlier attempt that linked two objects ahead of the archive instead of building two libraries moved RSI by 8% and was discarded.)
Sweep
CORREL clears without an annotation now: the clearing test looks through
sqrt/fabs, the same transparency the firing side already used, and it is sound becausesqrt(v)is zero exactly whenvis. The inline arm's self-test is rebuilt around the shipped shape — it removes guards rather than injecting a divisor — and still pins all three halves: fires on a guard naming only an operand, clears when one names the divisor, silent when nothing bounds either.WILLR is untouched. Its change moves values on ordinary data, its batch tier came out ~8% faster, and the clamp ruling stands.
Green
Full C suite · generator
cargo test+clippy -D warnings· Rust clippy/doc/doctests/tests ·--codegenagainst the frozen oracle (161 passed, 0 failed in each of four backends) ·--xlang-hash(four backends bit-identical) ·--fuzz-064(0 failures) ·regen-check· source-list/cargo-lock/stream-retcode checks.Both value legs re-sabotage-proven against a build of the pre-#395 arithmetic: CCI came back
±Infon all 39 bars, CORRELNaN, -1, +1.https://claude.ai/code/session_01GesMuivQUwX4xHr4hEtnS8