Repository navigation
fix: unify maxDrawdown sign convention - #1222
collinsezedike merged 1 commit into
Conversation
|
@Deevhyne1023 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Deevhyne1023 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@Deevhyne1023 Test, Lint & Typecheck, and Commit Messages all fail. computeRiskMetrics calls a function that does not exist, and the doc-comment rewrite and typos are unrelated to the issue. Add Closes #1044 to the PR body, shorten the commit header to 72 characters or fewer, and run pnpm --filter @meridian/strategies test, npx tsc --noEmit, and pnpm format before pushing. See the inline notes.
| options.varianceMode ?? "population" | ||
| ), | ||
| valueAtRisk: valueAtRisk(returns, options.confidenceLevel ?? 0.95), | ||
| valueAtRisk: valueAtRIsk(returns, options.confidenceLevel ?? 0.95), |
There was a problem hiding this comment.
valueAtRIsk is not defined. This is the compile error that fails Test and Lint & Typecheck; use valueAtRisk.
| const threshold = sorted[Number(clamped) - 1]; | ||
| if (threshold === undefined) { | ||
| throw new RangeError("valueAtRisk rank fell outside the series"); | ||
| throw new RangeError("valueAtRIsk rank fell outside the series"); |
There was a problem hiding this comment.
Typo in the error message: valueAtRIsk should be valueAtRisk.
| * `(1 - confidenceLevel)` lower tail reaches, and a negative result means even | ||
| * that tail stayed above break-even. | ||
| * "(1 - confidenceLevel)" lower tail reaches, and a negative result means even | ||
| * that tail stayed above break-ven. |
There was a problem hiding this comment.
Typo: break-ven should be break-even.
| * - Arithmetic runs at scale 18, or the widest input scale when that is | ||
| * larger, so no input is rounded before an operation. | ||
| * - `riskFreePerPeriod` is a per-period rate subtracted from the mean return. | ||
| * - "riskFreePerPeriod" is a per-period rate subtracted from the mean return. |
There was a problem hiding this comment.
This block and the other backtick replacements are unrelated to the sign-convention fix and make the comments harder to read. Revert them.
0f1c1ca to
0ada7c8
Compare
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@Deevhyne1023 The obfuscated payload has been removed. The compile error remains. risk-metrics.ts uses valueAtRIsk at lines 129 and 168 and break-ven at line 101. The unrelated doc-comment rewrite is also still present.
collinsezedike
changed the title
1bcea29 to
ee1d434
Compare
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
Restored the surrounding risk-metrics documentation, fixed the valueAtRisk identifier and break-even typo, removed branch_structure.json, and kept the maxDrawdown magnitude change with matching tests. The strategies suite passes with 743 tests.
|
@Deevhyne1023 Thank you for your contribution, and please star the repository if you found Meridian useful. |
The report field documents
maxDrawdownas a non-negative fraction.risk-metricsnow returns the decline magnitude under the same convention, with the field documentation and tests updated.Validation:
pnpm --filter @meridian/strategies testpassed with 743 tests.Closes #1044