Conversation
π WalkthroughWalkthroughThis pull request adds two new technical indicators to the trend package: Slow Stochastic and Schaff Trend Cycle (STC). The changes include new generic type implementations, constructor and compute methods, an additional method on the existing Apo type, documentation updates, and corresponding tests. Changes
Sequence DiagramssequenceDiagram
participant Client
participant SlowStochastic
participant MovingMinMax
participant SMA
participant Output
Client->>SlowStochastic: Compute(values)
activate SlowStochastic
SlowStochastic->>MovingMinMax: Buffer and track min/max over Period
activate MovingMinMax
MovingMinMax->>SlowStochastic: emit min/max windows
deactivate MovingMinMax
SlowStochastic->>SlowStochastic: Calculate FastK = 100 * (value - min) / (max - min)
SlowStochastic->>SMA: Smooth FastK with KPeriod
activate SMA
SMA->>SlowStochastic: emit SlowK
deactivate SMA
SlowStochastic->>SMA: Smooth SlowK with DPeriod
activate SMA
SMA->>SlowStochastic: emit SlowD
deactivate SMA
SlowStochastic->>Output: return (SlowK channel, SlowD channel)
deactivate SlowStochastic
Client->>Output: consume results
sequenceDiagram
participant Client
participant Stc
participant Apo
participant MovingMinMax
participant Stochastic
participant Output
Client->>Stc: Compute(values)
activate Stc
Stc->>Apo: Feed buffered values
activate Apo
Apo->>Stc: emit MACD values
deactivate Apo
Stc->>MovingMinMax: Track min/max of MACD over KPeriod
activate MovingMinMax
MovingMinMax->>Stc: emit min/max windows
deactivate MovingMinMax
Stc->>Stochastic: Calculate stochastic from MACD range
activate Stochastic
Stochastic->>Stc: emit stochastic result
deactivate Stochastic
Stc->>Stc: Scale result by 100
Stc->>Output: return STC channel
deactivate Stc
Client->>Output: consume results
Estimated code review effortπ― 4 (Complex) | β±οΈ ~65 minutes Poem
π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing Touches
π§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Reportβ Patch coverage is
β Your project status has failed because the head coverage (89.82%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
==========================================
- Coverage 89.87% 89.82% -0.05%
==========================================
Files 196 198 +2
Lines 5381 5473 +92
==========================================
+ Hits 4836 4916 +80
- Misses 477 491 +14
+ Partials 68 66 -2 β View full report in Codecov by Sentry. π New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
π§Ή Nitpick comments (2)
trend/stc.go (1)
92-135:Stochasticfield used only for configuration, not forCompute.The
Stc.Computemethod manually re-implements the stochastic calculation (movingMin, movingMax, divide, SMA) instead of delegating tos.Stochastic.Compute(). TheStochasticfield is only used to access.Period,.Sma, and.IdlePeriod(). This creates a maintenance risk: ifStochastic.Computelogic changes,Stcwon't pick up the changes. Consider either delegating tos.Stochastic.Compute()and working with its returned channels, or replacing the*Stochastic[T]field with the raw configuration values (period, smaPeriod) to make the decoupling explicit.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trend/stc.go` around lines 92 - 135, Stc.Compute currently re-implements the stochastic math instead of using the existing Stochastic implementation; refactor either to call s.Stochastic.Compute(...) to obtain the stochastic output channels (kValues and d) and use those channels (respecting s.Stochastic.IdlePeriod() and s.Stochastic.Sma.IdlePeriod()) in the later macd/k/d math, or replace the s.Stochastic field with explicit config values (Period, SmaPeriod, IdlePeriod) and remove the local movingMin/movingMax/Sma code; update references in Stc.Compute (where movingMin, movingMax, kValues, d, kDuplicate, and related helper.Skip calls are used) to use the stochastic Compute output or the new config fields so the STC logic delegates to the canonical Stochastic implementation.trend/slow_stochastic.go (1)
79-85: Division by zero when all values in the window are equal.When
highest == lowest(flat price window),helper.Dividewill produce+InforNaNfor float types. This is consistent with the existingStochastic[T].Computebehavior in this codebase, but worth noting as a known edge case.π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trend/slow_stochastic.go` around lines 79 - 85, The current computation for fastK can divide by zero when highest == lowest (flat window) causing +Inf/NaN; update the logic in the fastK calculation (around fastK, helper.Divide, highest, lowest, skipped, lowestSplice) to check if highest == lowest first and handle that case (e.g., set fastK to 0.0 or another defined fallback) instead of performing the Divide; mirror the safe behavior used by Stochastic[T].Compute so the code returns a stable finite value rather than Inf/NaN.
π€ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@trend/stc_test.go`:
- Around line 54-78: TestStcFull is only a smoke test that asserts len(slice)>0
and never validates numeric results or other constructors/behaviors; update
tests to (1) add assertions using helper.CheckEquals to compare computed STC
values against expected K/D/Value columns from testdata/stochastic.csv using the
existing TestStcFull flow (read CSV via helper.ReadFromCsvFile, map values, call
Stc.Compute on trend.NewStcWithPeriod/ trend.NewStc()), (2) add separate tests
that instantiate default constructor NewStc() and
NewSlowStochasticWithPeriod(...) and assert their IdlePeriod() returns the
expected idle length and that Compute produces identical results where
applicable, and (3) exercise SlowStochastic.IdlePeriod and Stc.IdlePeriod
methods; reference the functions/types NewStcWithPeriod, NewStc,
NewSlowStochasticWithPeriod, Stc.Compute, SlowStochastic, IdlePeriod, and
helper.CheckEquals when locating code to change.
- Around line 15-52: TestSlowStochastic currently only checks that a value is
produced and uses a nondeterministic select that can return on first channel
close leaving the other channel/driving goroutine dangling; update the test to
drain actualK and actualD deterministically (do not use a select over both) by
converting each channel to a slice (use helper.ChanToSlice or explicit drains),
load expected K and D values from the existing CSV fixture, apply
helper.RoundDigits as needed, and assert equality with helper.CheckEquals
against trend.NewSlowStochastic()[s].Computeβs outputs (actualK, actualD);
ensure both channels are fully consumed so no goroutine leaks and use
helper.CheckEquals for all value assertions to meet the coding guidelines.
In `@trend/stc.go`:
- Around line 128-134: The expression computing STC can divide by zero when
Subtract(d, kValues) == 0; update the logic around MultiplyBy/Divide/Subtract
(references: macdForStc, kValues, d, helper.Subtract, helper.Divide,
helper.MultiplyBy) to guard the denominator: detect exact or near-zero
Subtract(d, kValues) (use a small epsilon like 1e-8) and handle that case
explicitly (e.g., return a sensible fallback such as 0 or the prior STC value,
or perform the division with epsilon) to avoid producing +Inf/NaN.
- Around line 25-38: The current STC doc/formula is incorrect: replace the
single-ratio formula in the Stc implementation with the standard Schaff Trend
Cycle two-stage stochastic smoothing, or explicitly mark this as a custom
variant; specifically, update the logic in the Stc type/methods (look for
functions/identifiers like Stc, Calculate/Compute, MACD, stochastic, %K/%D) so
that you compute MACD first, then apply a first stochastic to MACD to produce K1
and smooth it with an EMA (call it PF), then apply a second stochastic to PF to
produce K2 and smooth that with an EMA to produce STC, finally scale/clip STC to
the 0β100 range; if you intentionally want the previous ratio behavior instead,
change the comments/docstring to state this is a custom non-standard variant and
document its differences from the canonical two-stage stochastic STC.
---
Nitpick comments:
In `@trend/slow_stochastic.go`:
- Around line 79-85: The current computation for fastK can divide by zero when
highest == lowest (flat window) causing +Inf/NaN; update the logic in the fastK
calculation (around fastK, helper.Divide, highest, lowest, skipped,
lowestSplice) to check if highest == lowest first and handle that case (e.g.,
set fastK to 0.0 or another defined fallback) instead of performing the Divide;
mirror the safe behavior used by Stochastic[T].Compute so the code returns a
stable finite value rather than Inf/NaN.
In `@trend/stc.go`:
- Around line 92-135: Stc.Compute currently re-implements the stochastic math
instead of using the existing Stochastic implementation; refactor either to call
s.Stochastic.Compute(...) to obtain the stochastic output channels (kValues and
d) and use those channels (respecting s.Stochastic.IdlePeriod() and
s.Stochastic.Sma.IdlePeriod()) in the later macd/k/d math, or replace the
s.Stochastic field with explicit config values (Period, SmaPeriod, IdlePeriod)
and remove the local movingMin/movingMax/Sma code; update references in
Stc.Compute (where movingMin, movingMax, kValues, d, kDuplicate, and related
helper.Skip calls are used) to use the stochastic Compute output or the new
config fields so the STC logic delegates to the canonical Stochastic
implementation.
| func TestSlowStochastic(t *testing.T) { | ||
| resultChan := make(chan float64, 10) | ||
| go func() { | ||
| values := []float64{ | ||
| 100, 102, 101, 103, 105, 104, 106, 108, 107, 109, | ||
| 110, 111, 109, 112, 113, 115, 114, 116, 118, 117, | ||
| 119, 120, 118, 121, 122, 120, 123, 125, 124, 126, | ||
| } | ||
|
|
||
| input := helper.SliceToChan(values) | ||
| s := trend.NewSlowStochastic[float64]() | ||
| actualK, actualD := s.Compute(input) | ||
|
|
||
| for { | ||
| select { | ||
| case v, ok := <-actualK: | ||
| if !ok { | ||
| return | ||
| } | ||
| resultChan <- v | ||
| case v, ok := <-actualD: | ||
| if !ok { | ||
| return | ||
| } | ||
| resultChan <- v | ||
| case <-time.After(2 * time.Second): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| select { | ||
| case v := <-resultChan: | ||
| t.Logf("got value: %v", v) | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("timeout - no values produced") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestSlowStochastic lacks value assertions and has a flawed channel consumption pattern.
-
No correctness validation: The test only checks that at least one value is produced. It doesn't compare outputs against expected values using
helper.CheckEquals/helper.RoundDigits, which is required by coding guidelines. -
Nondeterministic
selectover K and D: Theselecton lines 28β42 returns on the first channel close, potentially leaving the other channel (and its upstream goroutines) dangling. IfactualKcloses beforeactualDis drained, goroutines writing toactualDwill leak.
Consider following the existing test pattern in the codebase: load expected values from a CSV, drain each channel independently (or use helper.ChanToSlice), and compare with helper.CheckEquals.
As per coding guidelines, "Use helper.CheckEquals for assertions in tests" and "Achieve 100% code coverage for all indicators".
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trend/stc_test.go` around lines 15 - 52, TestSlowStochastic currently only
checks that a value is produced and uses a nondeterministic select that can
return on first channel close leaving the other channel/driving goroutine
dangling; update the test to drain actualK and actualD deterministically (do not
use a select over both) by converting each channel to a slice (use
helper.ChanToSlice or explicit drains), load expected K and D values from the
existing CSV fixture, apply helper.RoundDigits as needed, and assert equality
with helper.CheckEquals against trend.NewSlowStochastic()[s].Computeβs outputs
(actualK, actualD); ensure both channels are fully consumed so no goroutine
leaks and use helper.CheckEquals for all value assertions to meet the coding
guidelines.
| func TestStcFull(t *testing.T) { | ||
| type Data struct { | ||
| Value float64 | ||
| K float64 | ||
| D float64 | ||
| } | ||
|
|
||
| input, err := helper.ReadFromCsvFile[Data]("testdata/stochastic.csv") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| values := helper.Map(input, func(d *Data) float64 { return d.Value }) | ||
|
|
||
| stc := trend.NewStcWithPeriod[float64](5, 10, 5, 3) | ||
| result := stc.Compute(values) | ||
|
|
||
| slice := helper.ChanToSlice(result) | ||
|
|
||
| t.Logf("STC generated %d values", len(slice)) | ||
|
|
||
| if len(slice) == 0 { | ||
| t.Fatal("no results generated") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestStcFull is a smoke test without value assertions.
The test reads a CSV with K and D columns but never uses them for validation. It only asserts len(slice) > 0. This doesn't verify computational correctness and won't catch regressions.
Additionally, there are no tests for:
NewStc()(default constructor)NewSlowStochasticWithPeriod()IdlePeriod()on bothSlowStochasticandStc
As per coding guidelines, "Achieve 100% code coverage for all indicators" and "Use helper.CheckEquals for assertions in tests".
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trend/stc_test.go` around lines 54 - 78, TestStcFull is only a smoke test
that asserts len(slice)>0 and never validates numeric results or other
constructors/behaviors; update tests to (1) add assertions using
helper.CheckEquals to compare computed STC values against expected K/D/Value
columns from testdata/stochastic.csv using the existing TestStcFull flow (read
CSV via helper.ReadFromCsvFile, map values, call Stc.Compute on
trend.NewStcWithPeriod/ trend.NewStc()), (2) add separate tests that instantiate
default constructor NewStc() and NewSlowStochasticWithPeriod(...) and assert
their IdlePeriod() returns the expected idle length and that Compute produces
identical results where applicable, and (3) exercise SlowStochastic.IdlePeriod
and Stc.IdlePeriod methods; reference the functions/types NewStcWithPeriod,
NewStc, NewSlowStochasticWithPeriod, Stc.Compute, SlowStochastic, IdlePeriod,
and helper.CheckEquals when locating code to change.
| // Stc represents the configuration parameters for calculating the | ||
| // Schaff Trend Cycle (STC) indicator. It combines MACD with | ||
| // stochastic oscillators to identify trend direction and potential | ||
| // entry points. | ||
| // | ||
| // EMA1 = EMA(values, fastPeriod) | ||
| // EMA2 = EMA(values, slowPeriod) | ||
| // MACD = EMA1 - EMA2 | ||
| // | ||
| // %K = Stochastic %K of MACD with kPeriod | ||
| // %D = Stochastic %D of MACD with dPeriod | ||
| // | ||
| // STC = 100 * (MACD - %K) / (%D - %K) | ||
| // |
There was a problem hiding this comment.
𧩠Analysis chain
π Web query:
Schaff Trend Cycle STC formula standard algorithm stochastic MACD
π‘ Result:
Schaff Trend Cycle (STC): βstochastic MACDβ standard algorithm
A common/reference implementation (Doug Schaffβs original style) is:
Inputs
TCLen= cycle length / stochastic lookback (often 10)MA1= fast EMA length (often 23)MA2= slow EMA length (often 50)Ξ±= smoothing factor (often 0.5)- Output is bounded 0β100; common signal levels: 25 / 75. [2]
Step-by-step formula
Let src be price (typically close).
1) MACD line (no signal line needed)
[
XMAC_t = EMA(src, MA1)_t - EMA(src, MA2)_t
]
(Trading code often just calls this MACD(c, MA1, MA2).) [1]
2) 1st stochastic of MACD (Fast %K), then exponential smoothing
[
L1_t = \min(XMAC_{t-TCLen+1 \dots t})
]
[
H1_t = \max(XMAC_{t-TCLen+1 \dots t})
]
[
K1_t =
\begin{cases}
100\cdot \dfrac{XMAC_t - L1_t}{H1_t - L1_t}, & H1_t \ne L1_t \
K1_{t-1}, & H1_t = L1_t
\end{cases}
]
[
PF_t = PF_{t-1} + \alpha,(K1_t - PF_{t-1})
]
This PF is the smoothed stochastic of the MACD. [1]
3) 2nd stochastic of that smoothed series, then exponential smoothing β STC
[
L2_t = \min(PF_{t-TCLen+1 \dots t})
]
[
H2_t = \max(PF_{t-TCLen+1 \dots t})
]
[
K2_t =
\begin{cases}
100\cdot \dfrac{PF_t - L2_t}{H2_t - L2_t}, & H2_t \ne L2_t \
K2_{t-1}, & H2_t = L2_t
\end{cases}
]
[
STC_t = STC_{t-1} + \alpha,(K2_t - STC_{t-1})
]
In the EasyLanguage reference, the final smoothed value is called PFF, and STC = PFF. [1]
Default parameters youβll often see
Many trading-platform guides use (TCLen, MA1, MA2) = (10, 23, 50) and interpret crosses of 25/75 as cycle/trend change signals. [2]
Sources: EasyLanguage reference code (Traders.com, Apr 2010) [1]; parameter/level conventions (Forex-Indicators.net) [2].
π Script executed:
cd /dev/null && find . -name "stc.go" -type f 2>/dev/null | head -5Repository: cinar/indicator
Length of output: 108
π Script executed:
cat -n trend/stc.go | head -150Repository: cinar/indicator
Length of output: 4587
STC implementation uses non-standard formula that does not match the Schaff Trend Cycle algorithm.
The documented formula STC = 100 * (MACD - %K) / (%D - %K) differs fundamentally from the standard Schaff Trend Cycle, which applies two rounds of stochastic smoothing with exponential averaging to the MACD line:
- First stochastic:
K1 = stochastic(MACD)then exponentially smoothed βPF - Second stochastic:
K2 = stochastic(PF)then exponentially smoothed βSTC
The result is naturally bounded 0β100.
The implemented formula calculates a ratio of differences (MACD - %K) / (%D - %K) which is unbounded and not a stochastic transformation. This produces a fundamentally different output than the standard algorithm. Verify the intended algorithm and either update the implementation to use proper double stochastic smoothing or document this as a custom variant.
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trend/stc.go` around lines 25 - 38, The current STC doc/formula is incorrect:
replace the single-ratio formula in the Stc implementation with the standard
Schaff Trend Cycle two-stage stochastic smoothing, or explicitly mark this as a
custom variant; specifically, update the logic in the Stc type/methods (look for
functions/identifiers like Stc, Calculate/Compute, MACD, stochastic, %K/%D) so
that you compute MACD first, then apply a first stochastic to MACD to produce K1
and smooth it with an EMA (call it PF), then apply a second stochastic to PF to
produce K2 and smooth that with an EMA to produce STC, finally scale/clip STC to
the 0β100 range; if you intentionally want the previous ratio behavior instead,
change the comments/docstring to state this is a custom non-standard variant and
document its differences from the canonical two-stage stochastic STC.
| return helper.MultiplyBy( | ||
| helper.Divide( | ||
| helper.Subtract(macdForStc, kValues), | ||
| helper.Subtract(d, kValues), | ||
| ), | ||
| 100, | ||
| ) |
There was a problem hiding this comment.
Division by zero when %D == %K.
When d equals kValues, the denominator Subtract(d, kValues) is zero, producing +Inf or NaN. Unlike the stochastic (value - min) / (max - min) pattern where a zero range is uncommon, %D converging to %K can happen in trending markets. Consider whether this needs a guard.
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trend/stc.go` around lines 128 - 134, The expression computing STC can divide
by zero when Subtract(d, kValues) == 0; update the logic around
MultiplyBy/Divide/Subtract (references: macdForStc, kValues, d, helper.Subtract,
helper.Divide, helper.MultiplyBy) to guard the denominator: detect exact or
near-zero Subtract(d, kValues) (use a small epsilon like 1e-8) and handle that
case explicitly (e.g., return a sensible fallback such as 0 or the prior STC
value, or perform the division with epsilon) to avoid producing +Inf/NaN.
2 participants
Describe Request
Trend: Schaff Trend Cycle (STC).
Fixed #53
Change Type
New indicator.
Summary by CodeRabbit
New Features
Documentation