Fix percentChange formula and Sharpe ratio calculation#82
Open
Fix percentChange formula and Sharpe ratio calculation#82
Conversation
- percentChange was dividing by end price instead of start price, producing incorrect percent change values. Fixed in both internal/calculator and internal/data packages. - Sharpe ratio was using daily (non-annualized) stdev in the denominator while the numerator used annualized return. Now consistently uses annualizedStdev. - Added unit tests for percentChange in both packages.
📝 WalkthroughWalkthroughTwo Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/calculator/factor_expression.service.go (1)
668-684:⚠️ Potential issue | 🟠 MajorFail fast on zero start price in
PricePercentChangeto avoid non-finite factor scores.
percentChange(Line 683) divides bystartwithout validation. In this package, you already return errors fromPricePercentChange, so use that path forstartPrice == 0.💡 Proposed fix
func (h factorMetricsHandler) PricePercentChange(pr *data.PriceCache, symbol string, start, end time.Time) (float64, error) { startPrice, err := pr.Get(symbol, start) if err != nil { return 0, err } + if startPrice == 0 { + return 0, factorMetricsMissingDataError{ + fmt.Errorf("pricePercentChange start price is zero for %s on %s", symbol, start.Format(time.DateOnly)), + } + } endPrice, err := pr.Get(symbol, end) if err != nil { return 0, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/calculator/factor_expression.service.go` around lines 668 - 684, The percentChange function currently divides by start without checking for zero; modify PricePercentChange (not percentChange) to detect startPrice == 0 after retrieving it from pr.Get and return a descriptive error (e.g., "start price is zero") instead of calling percentChange, so callers fail fast and avoid non-finite factor scores; leave percentChange unchanged or document it expects non-zero start.
🧹 Nitpick comments (2)
internal/calculator/percent_change_test.go (1)
8-55: Add astart == 0boundary test case.Given the new denominator logic, include one test that asserts behavior when the start value is zero (e.g., finite fallback or expected non-finite handling).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/calculator/percent_change_test.go` around lines 8 - 55, Add a boundary test case for start == 0 in Test_percentChange: append a test struct (e.g., name "start zero", end 100, start 0) to the tests slice and in the t.Run assertion check the actual behavior of percentChange(end, start) for non-finite results using math.IsInf/got or math.IsNaN(got); if the implementation instead uses a finite fallback, assert equality to that fallback value—target the percentChange function in percent_change_test.go.internal/data/percent_change_test.go (1)
8-49: Mirror the zero-denominator boundary test in this package as well.Please add a
start == 0case here too, so bothpercentChangeimplementations are protected against regression on the same edge condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/data/percent_change_test.go` around lines 8 - 49, Add a test case covering the zero-denominator boundary for percentChange: include a test where start == 0 (e.g., end: 100, start: 0) and assert the function doesn't panic and returns the same special value the other package expects (check for +Inf with math.IsInf(got, 1) or the matching behavior used elsewhere); update the Test_percentChange table and the t.Run assertion to handle comparing special values (use math.IsInf/math.IsNaN when expected is a non-finite value) so the test mirrors the zero-denominator protection in the other package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/calculator/metrics.service.go`:
- Line 57: Guard the Sharpe division by checking annualizedStdev before
computing sharpeRatio: replace the direct division sharpeRatio :=
annualizedReturn / annualizedStdev with a safe branch that treats zero (or
near-zero) volatility as a special case (e.g. if math.Abs(annualizedStdev) <
1e-12 then set sharpeRatio = 0.0) else compute sharpeRatio = annualizedReturn /
annualizedStdev; use math.Abs and a small epsilon to avoid floating-point edge
cases and update any callers/serializers to expect the chosen fallback value.
In `@internal/data/price.service.go`:
- Around line 99-101: The percentChange function divides by start
unconditionally; add a guard in percentChange to handle start == 0 (e.g., return
0.0 or another defined sentinel) before performing ((end - start) / start) * 100
to avoid producing non-finite values that break downstream stdev cache logic;
update percentChange to check start == 0 and return the chosen safe value, and
ensure callers that expect non-finite values still behave correctly.
---
Outside diff comments:
In `@internal/calculator/factor_expression.service.go`:
- Around line 668-684: The percentChange function currently divides by start
without checking for zero; modify PricePercentChange (not percentChange) to
detect startPrice == 0 after retrieving it from pr.Get and return a descriptive
error (e.g., "start price is zero") instead of calling percentChange, so callers
fail fast and avoid non-finite factor scores; leave percentChange unchanged or
document it expects non-zero start.
---
Nitpick comments:
In `@internal/calculator/percent_change_test.go`:
- Around line 8-55: Add a boundary test case for start == 0 in
Test_percentChange: append a test struct (e.g., name "start zero", end 100,
start 0) to the tests slice and in the t.Run assertion check the actual behavior
of percentChange(end, start) for non-finite results using math.IsInf/got or
math.IsNaN(got); if the implementation instead uses a finite fallback, assert
equality to that fallback value—target the percentChange function in
percent_change_test.go.
In `@internal/data/percent_change_test.go`:
- Around line 8-49: Add a test case covering the zero-denominator boundary for
percentChange: include a test where start == 0 (e.g., end: 100, start: 0) and
assert the function doesn't panic and returns the same special value the other
package expects (check for +Inf with math.IsInf(got, 1) or the matching behavior
used elsewhere); update the Test_percentChange table and the t.Run assertion to
handle comparing special values (use math.IsInf/math.IsNaN when expected is a
non-finite value) so the test mirrors the zero-denominator protection in the
other package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f96ab39-5560-47fc-ae47-b20318d97551
📒 Files selected for processing (5)
internal/calculator/factor_expression.service.gointernal/calculator/metrics.service.gointernal/calculator/percent_change_test.gointernal/data/percent_change_test.gointernal/data/price.service.go
| annualizedReturn := math.Pow((endValue/startValue), 1/numYears) - 1 | ||
|
|
||
| sharpeRatio := annualizedReturn / stdev | ||
| sharpeRatio := annualizedReturn / annualizedStdev |
There was a problem hiding this comment.
Guard zero-volatility cases before Sharpe division.
Line 57 can emit Inf/NaN when annualizedStdev == 0, and this value flows to persistence/API consumers.
💡 Proposed fix
- sharpeRatio := annualizedReturn / annualizedStdev
+ sharpeRatio := 0.0
+ if annualizedStdev != 0 {
+ sharpeRatio = annualizedReturn / annualizedStdev
+ }📝 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.
Suggested change
| sharpeRatio := annualizedReturn / annualizedStdev | |
| sharpeRatio := 0.0 | |
| if annualizedStdev != 0 { | |
| sharpeRatio = annualizedReturn / annualizedStdev | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/calculator/metrics.service.go` at line 57, Guard the Sharpe division
by checking annualizedStdev before computing sharpeRatio: replace the direct
division sharpeRatio := annualizedReturn / annualizedStdev with a safe branch
that treats zero (or near-zero) volatility as a special case (e.g. if
math.Abs(annualizedStdev) < 1e-12 then set sharpeRatio = 0.0) else compute
sharpeRatio = annualizedReturn / annualizedStdev; use math.Abs and a small
epsilon to avoid floating-point edge cases and update any callers/serializers to
expect the chosen fallback value.
Comment on lines
99
to
101
| func percentChange(end, start float64) float64 { | ||
| return ((end - start) / end) * 100 | ||
| return ((end - start) / start) * 100 | ||
| } |
There was a problem hiding this comment.
Add a zero-denominator guard in percentChange before computing returns.
Line 100 divides by start unconditionally. If start == 0, this yields a non-finite result and can corrupt the stdev cache computation path.
💡 Proposed fix
func percentChange(end, start float64) float64 {
+ if start == 0 {
+ return math.NaN()
+ }
return ((end - start) / start) * 100
}📝 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.
Suggested change
| func percentChange(end, start float64) float64 { | |
| return ((end - start) / end) * 100 | |
| return ((end - start) / start) * 100 | |
| } | |
| func percentChange(end, start float64) float64 { | |
| if start == 0 { | |
| return math.NaN() | |
| } | |
| return ((end - start) / start) * 100 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/data/price.service.go` around lines 99 - 101, The percentChange
function divides by start unconditionally; add a guard in percentChange to
handle start == 0 (e.g., return 0.0 or another defined sentinel) before
performing ((end - start) / start) * 100 to avoid producing non-finite values
that break downstream stdev cache logic; update percentChange to check start ==
0 and return the chosen safe value, and ensure callers that expect non-finite
values still behave correctly.
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
1 participant
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.
Summary
Two bugs in the financial metrics calculations:
1.
percentChangedivides by wrong value (both packages)The formula was:
This divides by the end price, but percent change should be relative to the start price:
Example: A stock going from $100 → $110 should be +10%, but the old formula computed
(110-100)/110 = 9.09%. This affected factor score calculations (viapricePercentChange) and the stdev cache (daily return calculations inprice.service.go).2. Sharpe ratio uses non-annualized stdev
The Sharpe ratio was computed as:
Since the numerator is annualized, the denominator should be too:
The
annualizedStdevwas already being calculated (stdev * sqrt(252)) and returned in the result struct — it just wasn't being used for Sharpe.Tests added
percentChangein bothinternal/calculatorandinternal/datapackagesSummary by CodeRabbit
Bug Fixes
Tests