Skip to content

refactor(trading-signals): share signal machinery via object-result base and fix BBANDS warm-up#1182

Open
bennycode wants to merge 1 commit into
mainfrom
refactor/object-result-indicator-base
Open

refactor(trading-signals): share signal machinery via object-result base and fix BBANDS warm-up#1182
bennycode wants to merge 1 commit into
mainfrom
refactor/object-result-indicator-base

Conversation

@bennycode

Copy link
Copy Markdown
Owner

Summary

BBANDS warm-up off-by-one: confirmed and fixed

BollingerBands only computed a result when pushUpdate returned a dropOut, which first happens at the (interval+1)-th input — while getRequiredInputs() returns interval and the SMA that defines the middle band stabilizes at exactly interval inputs. Verification against reference data:

  • JSON fixture (src/fixtures/BB/data.json, interval 20): the first non-null reference value sits at index 19 (the 20th input) — middle[19] = 74.3, which equals the mean of prices[0..19]. The old implementation emitted its first result at index 20.
  • Tulip Indicators fixture (BollingerBands.test.ts, interval 5): reference values start at index 4 — lower 80.53 / middle 82.43 / upper 84.32, which match the bands computed from the first five inputs (81.59, 81.06, 82.87, 83.0, 83.61; mean 82.426).
  • Both tests masked the missing first value by gating assertions on if (bb.isStable), so the reference values at the first index were silently skipped.

BollingerBands now computes from the full window once prices.length === interval, making getRequiredInputs() accurate. The tests assert the exact warm-up offset (getRequiredInputs() - 1), and a new test proves the middle band stabilizes in lockstep with an SMA of the same interval. BollingerBandsWidth (built on BBANDS) gains one earlier result accordingly; its new first value at index 19 (0.19, window closes[0..19]) is consistent with the TradingView-verified series.

Generic object-result indicator base

New TrendTechnicalIndicator<Result, Input, SignalState> in src/types/Indicator.ts mirrors TrendIndicatorSeries but is generic over the result type: previousResult caching via setResult(), abstract calculateSignalState(), and getSignal() returning {state, hasChanged}. It is exported through the existing types barrel.

  • MACD and BollingerBands now extend it; their hand-rolled #previousResult / #twoPreviousResult bookkeeping and duplicated calculateSignal / getSignal methods are deleted.
  • Public API unchanged: class names, MACDResult / BandsResult shapes, and the getSignal() return shape {state, hasChanged} are all stable.
  • Bidirectional replace() behavior is preserved; BollingerBands keeps a two-level price history so signal states are re-derived from the correct (result, price) pairing on replace. A strict bidirectional replace test for BBANDS results was added.

Behavior note: BBANDS signal semantics

BollingerBands.getSignal() previously compared the latest price against the bands from before that price entered the window. It now compares the latest price against the current bands (standard %B semantics, and the same comparison downstream code like MultiIndicatorConfluenceStrategy performs manually). All existing signal test scenarios produce identical states.

Follow-ups (out of scope)

StochasticOscillator and AccelerationBands still hand-roll the same signal machinery (calculateSignal + getSignal); ABANDS also computes only on dropOut and likely has the same warm-up off-by-one. Both are candidates for a follow-up migration onto TrendTechnicalIndicator.

Test plan

  • npm test in packages/trading-signals: 63 files / 455 tests pass, coverage 100% on all metrics (statements 1259/1259, branches 690/690, functions 232/232, lines 1235/1235)
  • npm run lint in packages/trading-signals passes
  • npx lerna run dist --scope trading-strategies --include-dependencies succeeds; additionally rebuilt all workspace packages via npm workspaces and ran the trading-strategies suite: 22 files / 248 tests pass

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors the trading-signals package to share common “object-shaped result” signal machinery across indicators, while also correcting a Bollinger Bands (BBANDS) warm-up off-by-one so the first computed bands align with the interval’s first full window.

Changes:

  • Fix BBANDS warm-up to emit the first bands result at interval inputs (instead of interval + 1) and update BBANDS/BBW tests to assert the correct warm-up offset.
  • Introduce TrendTechnicalIndicator<Result, Input, SignalState> as a generic base for composite/object results, providing previous-result caching and {state, hasChanged} signal reporting.
  • Migrate MACD and BollingerBands onto the new base, removing duplicated per-indicator previous-result + signal-change bookkeeping.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/trading-signals/src/types/Indicator.ts Adds TrendTechnicalIndicator base class for object-result indicators with shared signal-change tracking.
packages/trading-signals/src/volatility/BBANDS/BollingerBands.ts Fixes warm-up logic (compute at prices.length === interval) and adopts TrendTechnicalIndicator for signal machinery.
packages/trading-signals/src/momentum/MACD/MACD.ts Refactors MACD to extend TrendTechnicalIndicator, removing local previous-result/signal bookkeeping.
packages/trading-signals/src/volatility/BBANDS/BollingerBands.test.ts Strengthens tests around BBANDS warm-up offset, SMA alignment, replace behavior, and signal semantics.
packages/trading-signals/src/volatility/BBW/BollingerBandsWidth.test.ts Adjusts BBW expectations/offset to reflect BBANDS producing one earlier result.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants