fix: correct ATR divergence, the EMA seed, and RSI on a flat series - #105
Merged
Conversation
Three defects, all the same species: a faithful-looking port of the TA-Lib C source that dropped exactly one line. Atr never normalised its accumulator. The main output loop divided on the way out but carried the undivided value into the next bar, so the running average was multiplied by (period - 1) every bar. On a series whose true range is a constant 4.0 it returned 4.29, 4.41, 57.6, 749, 9737 - a factor of 13 per bar for period 14 - and reached +Infinity within a few hundred bars. The warm-up loop directly above it and the identical loop in Natr both normalise; only this one did not. TA_INT_EMA seeded itself low. `while (i-- > 0)` upstream became `i--; if (i <= 0) break;`, which accumulates period - 1 values while still dividing by period. EMA(20) over a constant series of 100 returned 95.476190 instead of 100. The seed is shared, so this reached Ema, Macd, MacdExt, MacdFix, Dema, Tema, T3, Apo, Ppo and Trix - and because the fast and slow legs were mis-seeded by different amounts, MACD read 0.53 on a series that never moved. Rsi divided by zero on a flat window. A window with no price change has an average gain and an average loss of exactly zero, so the ratio was 0/0 and every element came back NaN next to RetCode.Success. Upstream guards the same division and yields zero. All three division sites are now routed through one guarded helper, including the Metastock branch. The existing tests could not have caught any of this: they assert only that RetCode is Success. Each fix therefore comes with value-asserting tests verified to fail against the previous implementation - six of the seven new tests fail without these changes. The seventh, RSI over a rising series, is a control: it pins the legitimate zero-loss case at 100 so the new guard cannot quietly swallow it. Refs #63 Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Three defects found while validating the samples in #104. All three are in released packages, and all three are the same species: a faithful-looking port of the TA-Lib C source that dropped exactly one line.
None of them could have been caught by the existing tests, which assert only
RetCode == Success.1.
Atrnever normalised its accumulatorsrc/TechnicalAnalysis.Functions/Atr/TAFunc.cs— main output loop divided on the way out but carried the undivided value into the next bar:So the running average was multiplied by
period - 1every bar. On a series whose true range is a constant4.0, where ATR(14) must stay exactly4.0:Ratio is exactly
13.0 = period - 1. On 1500 bars, 1209 of 1486 outputs were+Infinity.The warm-up loop directly above it, and the identical loop in
Natr, both normalise. Only this one didn't.2.
TA_INT_EMAseeded itself lowUpstream
while (i-- > 0)becamei--; if (i <= 0) break;— which accumulatesperiod - 1values while still dividing byperiod. Testing the counter before decrementing rather than after is what loses the final term.EMA(20) over a constant series of
100returned 95.476190 instead of100.The seed is shared, so this reached
Ema,Macd,MacdExt,MacdFix,Dema,Tema,T3,Apo,PpoandTrix. Because the fast and slow legs are seeded with different periods, the error did not cancel: MACD read 0.53 on a series that never moved.3.
Rsidivided by zero on a flat windowA window with no price change has an average gain and an average loss of exactly zero, so
100 * gain / (gain + loss)was0/0— every elementNaN, returned next toRetCode.Successand a non-zero element count. A halted instrument reaches this.Upstream guards the same division and yields
0. All three division sites now route through one guarded helper, including the Metastock branch, which had the same unguarded divide.Verification
The important part, given how these survived: every new test was confirmed to fail against the previous implementation.
The six failures are exactly the six regression tests. The seventh new test — RSI over a monotonically rising series — passes both ways by design: it is a control pinning the legitimate zero-loss case at
100, so the new zero guard cannot quietly swallow it.All 225 pre-existing tests still pass, unchanged.
Follow-up
docs/guides/getting-started.md#10-known-library-defectsand the sample guides in #104 document all three as known defects. Once this merges, those sections should be removed or rewritten — I left them alone here to avoid a conflict with #104. Merge #104 first, then this.🤖 Generated with Claude Code