Reject the two point window Beta, Correlation and Covariance describe - #9728
Reject the two point window Beta, Correlation and Covariance describe#9728mkzung wants to merge 2 commits into
Conversation
All three carry the comment "assert the period is greater than two" and
raise "must be greater than 2 but was {period}", and all three test
period < 2, so a period of two reaches the calculation.
Two points always correlate perfectly. Fed ten bars of two series that are
not proportional, Correlation at period 2 returns 1 or -1 on every one of
its nine readings, while at period 3 the same series gives values between
-0.40 and 0.33. Beta and Covariance share the window.
ValueAtRisk states the same bound and tests period < 3.
There was a problem hiding this comment.
Thanks for the PR! Agree on Correlation: with a two point window Pearson is always ±1, so rejecting period < 3 there is the right fix and matches what the message already says.
Beta and Covariance are a different case though. With period = 2 the returns windows hold two points, and the variance / covariance of two returns are finite, well defined values, so both indicators work today for anyone using period 2. Tightening the guard to < 3 would break those users for no functional gain.
Could you please:
- keep the
Correlation.cschange and its test as is - revert the guard change in
Beta.csandCovariance.cs; if you'd like to clean up the mismatch there, fix the comment and exception message to say "greater than 1", which is what the code has always enforced - adjust the Beta / Covariance tests accordingly (e.g. assert period 1 throws with the corrected message, and period 2 is accepted). As written their comments talk about "the correlation of two points", which doesn't describe those indicators.
Thanks!
|
You are right, and the tests I wrote gave it away: they pasted the correlation argument into two indicators that do not compute a correlation. Done as asked.
Their tests now assert both halves: period 1 throws with the corrected message, period 2 is accepted. The comments say what each indicator actually needs, that two returns give a finite covariance, rather than repeating the correlation reasoning. 50 tests pass across |
2 participants
Description
Beta,CorrelationandCovariancenow testperiod < 3, which is the bound their comment and their message already state.Related Issue
Closes #9725
Motivation and Context
All three carry the comment "assert the period is greater than two", raise "must be greater than 2 but was {period}", and then test
period < 2, so a period of two reaches the calculation.Two points always correlate perfectly. Fed ten bars of two series that are not proportional,
Correlationat period 2 returns 1 or -1 on every one of its nine readings:BetaandCovarianceshare that window throughDualSymbolIndicator.ValueAtRiskstates the same bound and testsperiod < 3.Requires Documentation Change
No.
How Has This Been Tested?
One test per indicator, copied from
ValueAtRiskTests.PeriodBelowMinimumThrows.Keeping the tests and reverting the three source files:
with them in place, including the Spearman fixture:
Nothing in
Tests/,Algorithm.CSharp/orAlgorithm.Python/constructs any of the three with a period of 2, so no existing caller changes; the smallest literal in use is 3.Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>