Fix STC dividing by a near-zero denominator, producing -Inf/huge out-of-range swings - #428
Merged
Merged
Conversation
STC = 100*(MACD-%K)/(%D-%K) divided by a denominator that's frequently near zero (%K and %D are both stochastic-smoothed values of the same MACD series and track each other closely), so real-world input could yield -Inf or swings like 1435/-710 for an indicator documented to oscillate 0-100. The standard Schaff Trend Cycle algorithm instead applies the rolling-min/max stochastic normalization *twice* -- once to MACD to get %K1/%D1, then again to %D1 to get %K2/%D2 -- with STC = %D2. Both stages stay bounded in [0, 100] by construction, since they're each a range-normalized percentage smoothed by an SMA, not a division against another stochastic-smoothed value. TestStcFull's fixture was too short for the new, larger warm-up (two stochastic passes need more lead-in than one), so it now generates its own longer synthetic series instead of testdata/stochastic.csv. Fixes #425 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Depends on #430 (MovingSum NaN-poisoning fix) for STC to be fully usable on real data -- STC's math is correct here regardless, but a single flat MACD window anywhere in the series will otherwise cause an extended NaN run downstream until #430 also lands. See #430's description for the before/after numbers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #428 +/- ##
==========================================
- Coverage 92.32% 92.29% -0.03%
==========================================
Files 229 229
Lines 7280 7259 -21
==========================================
- Hits 6721 6700 -21
Misses 472 472
Partials 87 87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #425.
Summary
trend.Stc.ComputeWithContextdivided(MACD-%K)/(%D-%K)for its final line, per the doc comment'sSTC = 100 * (MACD - %K) / (%D - %K).%D-%Kis frequently near zero (both are stochastic-smoothed values of the same MACD series and track each other closely), so real-world input yields-Infand swings like1435/-710for an indicator documented to oscillate 0-100.Stochastic(rolling-min/max normalization + SMA) twice -- once to MACD to get%K1/%D1, then again to%D1to get%K2/%D2-- withSTC = %D2. Both stages stay bounded in [0, 100] by construction.%K1/%K2are discarded (STC only needs the doubly-smoothed%D), but each comes from the same internal duplicate fan-out as%D1/%D2insideStochastic.ComputeWithContext, so each is drained in its own goroutine (helper.DrainWithContext) -- an unread duplicate branch otherwise blocks the shared producer and stalls%D1/%D2too (this reintroduces the class of deadlock Fix Stc.ComputeWithContext output channel never closing #421 fixed if skipped).IdlePeriod()updated toApo.IdlePeriod() + 2*Stochastic.IdlePeriod()to match the now-doubled warm-up.TestStcFull's fixture (testdata/stochastic.csv, 19 rows, shared with two other tests) is too short for the new warm-up, so it now generates its own longer synthetic series instead.Note on remaining NaN
STC can still legitimately output
NaNwhen a stochastic stage's rolling window is exactly flat (a genuine 0/0). Separately, I found thattrend.MovingSum(which theSmainsideStochasticuses) doesn't recover from a singleNaNeven after it leaves the window -- filed and fixed separately as #427 / PR (fix/movingsum-nan-poisoning), since it's a shared primitive bug, not specific to STC. With that fix applied too, STC on a 251-day BRK-B series drops from 157/180 NaN outputs down to 36/180, with the rest being either the initial warm-up or a genuinely flat window at the very tail (no future data to recover into). Without it, STC's math is still correct here, just noisier in practice on real data until #427 lands.Test plan
go build ./...go test -race ./...(full repo, no regressions)go vet,gosec,staticcheck,reviveclean ontrend/asset/testdata/repository/brk-b.csv: output now bounded in [0, 100] (previously-Inf,1435,-710, etc.)TestStcFull(longer synthetic fixture) and existingTestStcDeadlock/TestStcSlowStochasticall pass🤖 Generated with Claude Code