Conversation
📝 WalkthroughWalkthroughThis PR implements the Connors RSI indicator by adding PercentRank helper functions and composing a Connors RSI indicator from RSI, Streak, and PercentRank sub-components. It also removes conductor documentation and archives while updating README links and inline documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (88.77%) 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 #321 +/- ##
==========================================
- Coverage 89.87% 88.77% -1.11%
==========================================
Files 200 202 +2
Lines 5531 5647 +116
==========================================
+ Hits 4971 5013 +42
- Misses 494 563 +69
- Partials 66 71 +5 ☔ 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 (3)
helper/percent_rank.go (1)
55-98:SortedPercentRankhas identical window semantics and the same idle-period concern asPercentRank.The "more accurate but slower" claim in the doc comment (line 54) is misleading — both functions count strictly-less-than values, so they should produce identical results for the same input. The sorted variant is actually slower (
O(n log n)sort + binary search per value vsO(n)linear scan). Consider clarifying the doc or providing a rationale for whenSortedPercentRankwould differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helper/percent_rank.go` around lines 55 - 98, SortedPercentRank currently implements identical window semantics to PercentRank but its doc comment incorrectly claims it's "more accurate but slower"; update the documentation to remove or correct that claim and explicitly state that SortedPercentRank produces the same strictly-less-than ranking as PercentRank but is slower (O(n log n) per item vs O(n)), or alternatively change SortedPercentRank to implement a different tie-handling/ranking rule if you intend a different result—modify the doc and/or function behavior accordingly and mention both SortedPercentRank and PercentRank by name so readers can find and compare the implementations.momentum/connors_rsi.go (2)
62-83: Silent fallback to defaults on invalid periods — consider returning an error or documenting this behavior.
NewConnorsRsiWithPeriodssilently replaces non-positive periods with defaults (lines 64–72). This could mask caller bugs. Other indicators in this codebase appear to trust caller input. At minimum, document this fallback behavior in the function's doc comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@momentum/connors_rsi.go` around lines 62 - 83, NewConnorsRsiWithPeriods silently replaces non‑positive rsiPeriod/streakRsiPeriod/percentRankPeriod with defaults (DefaultConnorsRsiRsiPeriod, DefaultConnorsRsiStreakRsiPeriod, DefaultConnorsRsiPercentRankPeriod), which can hide caller bugs; either change the constructor signature to return (*ConnorsRsi[T], error) and validate the three period args (return an error on non‑positive values) or keep the current behavior but update the function doc comment for NewConnorsRsiWithPeriods to explicitly state that non‑positive inputs are replaced by those specific default constants and why that fallback exists so callers are aware; locate and modify NewConnorsRsiWithPeriods, the DefaultConnorsRsi* constants, and any callers if you switch to the error‑return approach.
35-51: Type constraint useshelper.Floatinstead ofhelper.Number.The coding guidelines specify "Use generic type parameter
Tconstrained tohelper.Number". This type useshelper.Float, which may be intentional sinceT(1)/T(3)on line 110 requires floating-point division. If intentional, this is fine — just noting the deviation.As per coding guidelines, "Use generic type parameter
Tconstrained tohelper.Numberfor indicator types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@momentum/connors_rsi.go` around lines 35 - 51, Change the generic constraint on ConnorsRsi from helper.Float to helper.Number to follow the project guideline (update the type declaration for ConnorsRsi[T helper.Number]); then locate any places in this type's methods or initialization that assume float-only semantics (for example any expressions like T(1)/T(3) used in ConnorsRsi logic) and replace them with explicit, Number-safe operations — e.g., perform the division using a float64 conversion and convert back to T or use the helper package's conversion helpers so arithmetic remains correct while the generic constraint is helper.Number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@momentum/connors_rsi_test.go`:
- Around line 14-49: Add a new test file that exercises helper.PercentRank and
helper.SortedPercentRank to reach full coverage: write table-driven tests in
percent_rank_test.go that (1) verify normal operation over various periods
(e.g., period 3, 5, 10) by comparing expected numeric percent ranks for a sample
input series, (2) assert the early-return behavior when period <= 1 (ensure
function exits/returns empty or appropriate result), (3) test window-boundary
behavior by feeding sliding windows and verifying outputs align at the edges,
and (4) cross-check that PercentRank and SortedPercentRank produce consistent
results for identical inputs; reference the PercentRank and SortedPercentRank
constructor/func names in assertions and use helper utilities (e.g., creating
input slices, rounding/compare helpers) already used in other tests to keep
assertions deterministic.
- Around line 15-18: The Data test struct in momentum/connors_rsi_test.go is
missing CSV `header` struct tags; update the Data type declaration (fields Close
and ConnorsRsi) to include header tags (e.g., add `header:"Close"` and
`header:"ConnorsRsi"`) so the CSV mapping matches other tests like
td_sequential_test.go and the project coding guidelines.
In `@momentum/connors_rsi.go`:
- Around line 116-121: The IdlePeriod currently sums component idle values and
has a stale inline comment; change ConnorsRsi.IdlePeriod to compute the maximum
idle across parallel pipelines instead of a sum and update the comment
accordingly: return max(c.Rsi.IdlePeriod(),
c.Roc.IdlePeriod()+c.PercentRankPeriod) (the PercentRank pipeline’s idle is
Roc.Idle + PercentRankPeriod and typically dominates), and replace the outdated
arithmetic comment with a brief note that pipelines run in parallel and the idle
is the max of their idle times.
- Around line 86-114: The three component channels in ConnorsRsi.Compute are
misaligned (PercentRank is the slowest) so before combining with helper.Add you
must align faster streams using helper.Skip: call helper.Skip on rsis and
streakRsis so they drop the appropriate number of initial values to match
percentRanks (use the ConnorsRsi.Roc/PercentRankPeriod and the configured
RSI/StreakRsi periods to compute the skip counts), then combine the three
aligned streams with helper.Add and helper.MultiplyBy as before; also update the
IdlePeriod() comment to correctly compute the combined idle period from the
configured subperiods (e.g., 1 + ConnorsRsi.Rsi.Period (3) +
ConnorsRsi.PercentRankPeriod (100) = 104 with default values) and remove the
incorrect “RMA period 14” text.
---
Nitpick comments:
In `@helper/percent_rank.go`:
- Around line 55-98: SortedPercentRank currently implements identical window
semantics to PercentRank but its doc comment incorrectly claims it's "more
accurate but slower"; update the documentation to remove or correct that claim
and explicitly state that SortedPercentRank produces the same strictly-less-than
ranking as PercentRank but is slower (O(n log n) per item vs O(n)), or
alternatively change SortedPercentRank to implement a different
tie-handling/ranking rule if you intend a different result—modify the doc and/or
function behavior accordingly and mention both SortedPercentRank and PercentRank
by name so readers can find and compare the implementations.
In `@momentum/connors_rsi.go`:
- Around line 62-83: NewConnorsRsiWithPeriods silently replaces non‑positive
rsiPeriod/streakRsiPeriod/percentRankPeriod with defaults
(DefaultConnorsRsiRsiPeriod, DefaultConnorsRsiStreakRsiPeriod,
DefaultConnorsRsiPercentRankPeriod), which can hide caller bugs; either change
the constructor signature to return (*ConnorsRsi[T], error) and validate the
three period args (return an error on non‑positive values) or keep the current
behavior but update the function doc comment for NewConnorsRsiWithPeriods to
explicitly state that non‑positive inputs are replaced by those specific default
constants and why that fallback exists so callers are aware; locate and modify
NewConnorsRsiWithPeriods, the DefaultConnorsRsi* constants, and any callers if
you switch to the error‑return approach.
- Around line 35-51: Change the generic constraint on ConnorsRsi from
helper.Float to helper.Number to follow the project guideline (update the type
declaration for ConnorsRsi[T helper.Number]); then locate any places in this
type's methods or initialization that assume float-only semantics (for example
any expressions like T(1)/T(3) used in ConnorsRsi logic) and replace them with
explicit, Number-safe operations — e.g., perform the division using a float64
conversion and convert back to T or use the helper package's conversion helpers
so arithmetic remains correct while the generic constraint is helper.Number.
| func TestConnorsRsi(t *testing.T) { | ||
| type Data struct { | ||
| Close float64 | ||
| ConnorsRsi float64 | ||
| } | ||
|
|
||
| input, err := helper.ReadFromCsvFile[Data]("testdata/connors_rsi.csv") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| inputs := helper.Duplicate(input, 2) | ||
| closings := helper.Map(inputs[0], func(d *Data) float64 { return d.Close }) | ||
| expected := helper.Map(inputs[1], func(d *Data) float64 { return d.ConnorsRsi }) | ||
|
|
||
| connorsRsi := momentum.NewConnorsRsi[float64]() | ||
| actual := connorsRsi.Compute(closings) | ||
| actual = helper.RoundDigits(actual, 2) | ||
|
|
||
| expected = helper.Skip(expected, connorsRsi.IdlePeriod()) | ||
|
|
||
| err = helper.CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| func TestConnorsRsiString(t *testing.T) { | ||
| connorsRsi := momentum.NewConnorsRsi[float64]() | ||
| expected := "ConnorsRSI(3, 2, 100)" | ||
| actual := connorsRsi.String() | ||
|
|
||
| if actual != expected { | ||
| t.Fatalf("expected %v, got %v", expected, actual) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for helper/percent_rank.go — Codecov shows 0%.
The PR adds PercentRank and SortedPercentRank in helper/percent_rank.go with zero direct test coverage. The coding guidelines require 100% coverage. A dedicated helper/percent_rank_test.go should be added covering:
- Normal operation with various periods
- Edge case:
period <= 1(early close) - Window boundary behavior
- Both
PercentRankandSortedPercentRankproducing consistent results
As per coding guidelines, "Achieve 100% code coverage for all indicators".
Would you like me to generate a helper/percent_rank_test.go or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/connors_rsi_test.go` around lines 14 - 49, Add a new test file that
exercises helper.PercentRank and helper.SortedPercentRank to reach full
coverage: write table-driven tests in percent_rank_test.go that (1) verify
normal operation over various periods (e.g., period 3, 5, 10) by comparing
expected numeric percent ranks for a sample input series, (2) assert the
early-return behavior when period <= 1 (ensure function exits/returns empty or
appropriate result), (3) test window-boundary behavior by feeding sliding
windows and verifying outputs align at the edges, and (4) cross-check that
PercentRank and SortedPercentRank produce consistent results for identical
inputs; reference the PercentRank and SortedPercentRank constructor/func names
in assertions and use helper utilities (e.g., creating input slices,
rounding/compare helpers) already used in other tests to keep assertions
deterministic.
| type Data struct { | ||
| Close float64 | ||
| ConnorsRsi float64 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual connors_rsi_test.go file around lines 15-18
head -25 momentum/connors_rsi_test.go | cat -nRepository: cinar/indicator
Length of output: 745
🏁 Script executed:
# Check other test files in momentum directory to see if they use header tags
rg -n 'header:' --type=go -g '*_test.go' momentum/Repository: cinar/indicator
Length of output: 455
🏁 Script executed:
# Look at the helper.ReadFromCsvFile function to understand tag requirements
rg -A 20 'func ReadFromCsvFile' helper/Repository: cinar/indicator
Length of output: 4055
🏁 Script executed:
# Look at the CSV struct and how it handles field mapping
rg -B 5 -A 30 'type Csv\[' helper/csv.go | head -60Repository: cinar/indicator
Length of output: 991
🏁 Script executed:
# Check if there are other test files without header tags
rg -n 'type Data struct' --type=go -g '*_test.go' | head -20Repository: cinar/indicator
Length of output: 916
🏁 Script executed:
# Look for ReadFromFile implementation to see how it handles mapping
rg -B 3 -A 15 'func.*ReadFromFile' helper/csv.goRepository: cinar/indicator
Length of output: 586
🏁 Script executed:
# Check how csvColumn is populated - look for NewCsv implementation
rg -B 5 -A 40 'func NewCsv\[' helper/csv.goRepository: cinar/indicator
Length of output: 1148
🏁 Script executed:
# Check one of the test files that doesn't have header tags to see if it works
head -30 trend/cci_test.goRepository: cinar/indicator
Length of output: 789
🏁 Script executed:
# Search for how the header tag is used in the CSV implementation
rg -n 'header:' helper/csv.go | head -20Repository: cinar/indicator
Length of output: 41
Missing CSV header struct tags on the test data structure.
The Data struct should include header tags for CSV field mapping to match coding guidelines:
Proposed fix
type Data struct {
- Close float64
- ConnorsRsi float64
+ Close float64 `header:"Close"`
+ ConnorsRsi float64 `header:"ConnorsRsi"`
}Per coding guidelines: "Test data structures must include CSV header tags (e.g., Close float64 \header:"Close"`)". This matches the pattern used in other test files like momentum/td_sequential_test.go`.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Data struct { | |
| Close float64 | |
| ConnorsRsi float64 | |
| } | |
| type Data struct { | |
| Close float64 `header:"Close"` | |
| ConnorsRsi float64 `header:"ConnorsRsi"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/connors_rsi_test.go` around lines 15 - 18, The Data test struct in
momentum/connors_rsi_test.go is missing CSV `header` struct tags; update the
Data type declaration (fields Close and ConnorsRsi) to include header tags
(e.g., add `header:"Close"` and `header:"ConnorsRsi"`) so the CSV mapping
matches other tests like td_sequential_test.go and the project coding
guidelines.
| func (c *ConnorsRsi[T]) Compute(closings <-chan T) <-chan T { | ||
| cs := helper.Duplicate(closings, 3) | ||
|
|
||
| cs[0] = helper.Buffered(cs[0], 100) | ||
| cs[1] = helper.Buffered(cs[1], 100) | ||
| cs[2] = helper.Buffered(cs[2], 100) | ||
|
|
||
| // Component 1: RSI on closing prices | ||
| rsis := c.Rsi.Compute(cs[0]) | ||
|
|
||
| // Component 2: RSI on streak length | ||
| streaks := c.Streak.Compute(cs[1]) | ||
| streakRsis := c.StreakRsi.Compute(streaks) | ||
|
|
||
| // Component 3: PercentRank of ROC | ||
| rocs := c.Roc.Compute(cs[2]) | ||
| percentRanks := helper.PercentRank(rocs, c.PercentRankPeriod) | ||
|
|
||
| // Combine: average of three components | ||
| result := helper.MultiplyBy( | ||
| helper.Add( | ||
| helper.Add(rsis, streakRsis), | ||
| percentRanks, | ||
| ), | ||
| T(1)/T(3), | ||
| ) | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check IdlePeriod implementations for RSI and Roc
echo "=== RSI IdlePeriod implementation ==="
ast-grep --pattern $'func ($_ *Rsi[$_]) IdlePeriod() int {
$$$
}'
echo -e "\n=== Roc IdlePeriod implementation ==="
ast-grep --pattern $'func ($_ *Roc[$_]) IdlePeriod() int {
$$$
}'
echo -e "\n=== PercentRank function signature and idle period ==="
rg -n "func PercentRank" --type=go -A5
echo -e "\n=== Examples of stream synchronization (Shift, Skip, CommonPeriod) ==="
rg -n "helper\.(Shift|Skip|SyncPeriod|CommonPeriod)" --type=go -C2 -g '!*_test.go' | head -60Repository: cinar/indicator
Length of output: 3139
🏁 Script executed:
#!/bin/bash
# Check ConnorsRsi's actual IdlePeriod calculation and test
echo "=== ConnorsRsi IdlePeriod implementation ==="
rg -n "func.*ConnorsRsi.*IdlePeriod" --type=go -A3
echo -e "\n=== ConnorsRsi test data and expectations ==="
rg -n "TestConnorsRsi" --type=go -A20 | head -40
echo -e "\n=== Check Streak component implementation ==="
rg -n "type Streak\[" --type=go -A10Repository: cinar/indicator
Length of output: 3066
🏁 Script executed:
#!/bin/bash
# Check if helper.Add handles streams with different lengths and how
echo "=== helper.Add implementation ==="
rg -n "func Add\[" --type=go -A20 | head -40
echo -e "\n=== PercentRank full implementation ==="
rg -n "func PercentRank\[" --type=go -A30 | head -50Repository: cinar/indicator
Length of output: 1715
🏁 Script executed:
#!/bin/bash
# Check RMA IdlePeriod implementation
echo "=== RMA/Rma IdlePeriod implementation ==="
rg -n "func.*Rma.*IdlePeriod" --type=go -A2
# Check default Rma period used in Rsi
echo -e "\n=== How Rsi initializes its Rma field ==="
rg -n "type Rsi\[" --type=go -A15
# Check Operate (used by Add) to understand how it handles stream length mismatches
echo -e "\n=== Operate function (base of Add) ==="
rg -n "func Operate\[" --type=go -A25 | head -50
# Check if test data size might be too small to reveal the issue
echo -e "\n=== Test data row count ==="
wc -l momentum/connors_rsi_test.go
find . -name "connors_rsi.csv" -exec wc -l {} \;Repository: cinar/indicator
Length of output: 2033
🏁 Script executed:
#!/bin/bash
# Check the actual Rma field initialization in Rsi
echo "=== Rsi struct and constructor ==="
rg -n "type Rsi\[" --type=go -A20
echo -e "\n=== NewRsiWithPeriod function ==="
rg -n "func NewRsiWithPeriod" --type=go -A10Repository: cinar/indicator
Length of output: 1809
🏁 Script executed:
#!/bin/bash
# Check Streak.IdlePeriod() explicitly
echo "=== Streak.IdlePeriod() implementation ==="
rg -n "func.*Streak.*IdlePeriod" --type=go -A2
# Check DefaultConnorsRsiRsiPeriod and other defaults
echo -e "\n=== ConnorsRsi default constants ==="
rg -n "Default.*Connors" --type=go
# Check MapWithPrevious to confirm it doesn't add idle period
echo -e "\n=== MapWithPrevious implementation ==="
rg -n "func MapWithPrevious" --type=go -A15 | head -30
# Look at how helper.Skip is used to understand synchronization
echo -e "\n=== Check if there's a synchronization step we might have missed ==="
rg -n "ConnorsRsi\[T\].*Compute" --type=go -A40 | head -50Repository: cinar/indicator
Length of output: 4276
🏁 Script executed:
#!/bin/bash
# Verify the actual idle period values by checking each component
echo "=== Verify: RSI with default period ==="
rg -n "DefaultRsiPeriod" --type=go
echo -e "\n=== Verify: StreakRsi period in ConnorsRsi ==="
rg -n "NewStreakRsi\|DefaultStreakRsi" --type=go
echo -e "\n=== Full ConnorsRsi struct and NewConnorsRsi ==="
rg -n "type ConnorsRsi\[" --type=go -A30Repository: cinar/indicator
Length of output: 2360
🏁 Script executed:
#!/bin/bash
# Let me verify Streak actually returns one value per input (no additional idle)
echo "=== Full Streak.Compute implementation ==="
rg -n "func.*Streak.*Compute" --type=go -A25
# Check what happens with stream 3 more carefully - total idle
echo -e "\n=== PercentRank's actual buffering behavior ==="
cat helper/percent_rank.go | head -50Repository: cinar/indicator
Length of output: 2600
🏁 Script executed:
#!/bin/bash
# Trace exact idle periods to verify the math
python3 << 'EOF'
# Idle period calculations with default ConnorsRsi periods
rsi_period = 3
streak_rsi_period = 2
percent_rank_period = 100
roc_period = 1 # hardcoded in Compute
# RMA idle = period - 1
# RSI idle = rma.idle + 1 = (period-1) + 1 = period
stream1_idle = rsi_period # Rsi(3)
stream2_idle = 1 + streak_rsi_period # Streak + StreakRsi: Change(1) + Rsi(2)
stream3_idle = roc_period + percent_rank_period # Roc(1) + PercentRank(100)
print(f"Stream 1 (RSI on closings): {stream1_idle} idle, produces N-{stream1_idle} values")
print(f"Stream 2 (Streak → StreakRsi): {stream2_idle} idle, produces N-{stream2_idle} values")
print(f"Stream 3 (Roc → PercentRank): {stream3_idle} idle, produces N-{stream3_idle} values")
print()
print(f"Bottleneck stream (3) produces only N-{stream3_idle} values")
print(f"Streams 1&2 produce N-{stream1_idle} values")
print(f"Temporal misalignment: {stream3_idle - stream1_idle} time steps")
print()
print(f"IdlePeriod() returns: {roc_period} + {rsi_period} + {percent_rank_period} = {roc_period + rsi_period + percent_rank_period}")
print(f"But actual max idle needed: {stream3_idle}")
print(f"Discrepancy in comment: claims '1+3+14+100=118' but code calculates '1+3+100=104' and actual needed is {stream3_idle}")
EOFRepository: cinar/indicator
Length of output: 533
Critical: The three component streams are not synchronized—they produce output at different time indices.
Stream idle periods and output counts:
- Stream 1 (RSI on closings): 3 values consumed → N−3 values produced
- Stream 2 (Streak → StreakRSI): 1+2=3 values consumed → N−3 values produced
- Stream 3 (ROC → PercentRank(100)): 1+100=101 values consumed → N−101 values produced
When helper.Add combines these via Operate, it reads from both channels until either closes, then drains and stops. The result is N−101 values where position 0 contains RSI/StreakRSI from time index 3 averaged with PercentRank from time index 101 (98-step temporal offset). The three components are misaligned by nearly 100 time steps and should not be averaged together.
Use helper.Skip on the faster streams (as done in volatility/po.go and trend/mass_index.go) to align them to the slowest stream before combining.
Also, the IdlePeriod() comment (line 118) is factually incorrect: it references "RMA period 14" (the global default RSI period, not Connors RSI's configured period 3) and states "= 118" when the actual calculation is 1+3+100=104 with default periods. Correct the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/connors_rsi.go` around lines 86 - 114, The three component channels
in ConnorsRsi.Compute are misaligned (PercentRank is the slowest) so before
combining with helper.Add you must align faster streams using helper.Skip: call
helper.Skip on rsis and streakRsis so they drop the appropriate number of
initial values to match percentRanks (use the ConnorsRsi.Roc/PercentRankPeriod
and the configured RSI/StreakRsi periods to compute the skip counts), then
combine the three aligned streams with helper.Add and helper.MultiplyBy as
before; also update the IdlePeriod() comment to correctly compute the combined
idle period from the configured subperiods (e.g., 1 + ConnorsRsi.Rsi.Period (3)
+ ConnorsRsi.PercentRankPeriod (100) = 104 with default values) and remove the
incorrect “RMA period 14” text.
| // IdlePeriod is the initial period that Connors RSI won't yield any results. | ||
| func (c *ConnorsRsi[T]) IdlePeriod() int { | ||
| // ROC period 1 + RSI period 3 + RMA period 14 + PercentRank period 100 | ||
| // = 1 + 3 + 14 + 100 = 118 | ||
| return c.Roc.IdlePeriod() + c.Rsi.IdlePeriod() + c.PercentRankPeriod | ||
| } |
There was a problem hiding this comment.
Incorrect inline comment and likely incorrect IdlePeriod calculation.
Two issues:
-
Stale comment (line 118–119): References "RMA period 14" which is the default
Rsiperiod, not the Connors RSI's RSI period of 3. The arithmetic "1 + 3 + 14 + 100 = 118" doesn't match the code's return value either. -
Sum vs max: The three pipelines run in parallel (not in series), so the correct idle period should be
max(stream1_idle, stream2_idle, stream3_idle), not the sum of component idle periods. Since stream 3 dominates (Roc idle + PercentRankPeriod), the idle period should approximate1 + PercentRankPeriod, not the sum of all components.
Proposed fix (after stream synchronization is resolved)
// IdlePeriod is the initial period that Connors RSI won't yield any results.
func (c *ConnorsRsi[T]) IdlePeriod() int {
- // ROC period 1 + RSI period 3 + RMA period 14 + PercentRank period 100
- // = 1 + 3 + 14 + 100 = 118
- return c.Roc.IdlePeriod() + c.Rsi.IdlePeriod() + c.PercentRankPeriod
+ // The idle period is the maximum of the three parallel pipelines:
+ // 1. RSI idle period
+ // 2. Streak idle (1) + StreakRsi idle period
+ // 3. Roc idle period + PercentRank period
+ rsiIdle := c.Rsi.IdlePeriod()
+ streakIdle := c.Streak.IdlePeriod() + c.StreakRsi.IdlePeriod()
+ rocPrIdle := c.Roc.IdlePeriod() + c.PercentRankPeriod
+ return max(rsiIdle, max(streakIdle, rocPrIdle))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/connors_rsi.go` around lines 116 - 121, The IdlePeriod currently
sums component idle values and has a stale inline comment; change
ConnorsRsi.IdlePeriod to compute the maximum idle across parallel pipelines
instead of a sum and update the comment accordingly: return
max(c.Rsi.IdlePeriod(), c.Roc.IdlePeriod()+c.PercentRankPeriod) (the PercentRank
pipeline’s idle is Roc.Idle + PercentRankPeriod and typically dominates), and
replace the outdated arithmetic comment with a brief note that pipelines run in
parallel and the idle is the max of their idle times.
2 participants
Describe Request
Connor RSI is added.
Fixed #256
Change Type
New indicator.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores