镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix(evaluate): return scalar beta with consistent covariance normalization - #2364

Open
Natsuki Komatsu (nkjapan) wants to merge 3 commits into
microsoft:mainfrom
nkjapan:main
Open

Natsuki Komatsu (nkjapan) wants to merge 3 commits into
microsoft:mainfrom
nkjapan:main

Conversation

@nkjapan

Copy link
Copy Markdown

Description

Fix get_beta in qlib/contrib/evaluate_portfolio.py to return the scalar ratio cov_r_b[0, 1] / cov_r_b[1, 1]. Taking both terms from the same covariance matrix also keeps their normalization consistent.

Add return-value documentation and 10 regression cases covering beta and its use in get_alpha.

Motivation and Context

The current implementation divides the entire 2×2 result of np.cov(r, b) by np.var(b), so it returns a matrix rather than a scalar. It also mixes the sample covariance's N - 1 denominator with the population variance's N denominator.

For five nonconstant observations with r == b, this produces a matrix filled with 1.25 instead of beta 1. The incorrect shape and value propagate into get_alpha; a benchmark compared with itself should have alpha 0.

How Has This Been Tested?

  • Added 10 cases in tests/misc/test_evaluate_portfolio.py: self-beta at three sample sizes, four affine benchmark scales, comparison with scipy.stats.linregress, and self-alpha at two risk-free rates. All 10 fail on the unmodified base and pass with this fix.
  • Ran both unmodified source CI workflows on commit 5dbc5086f9a14fe1c8d6d81fd40c9e1fe146148f: Windows, Ubuntu 22.04/24.04, and macOS 14/15, each with Python 3.8–3.12.
  • The added regressions passed in all 25 configurations. Normal pytest results were 112 passed/1 skipped per Linux configuration and 81 passed/1 skipped per Windows/macOS configuration; slow tests passed all 10 cases per configuration.
  • Existing test selection was preserved: RL tests run only on Linux, and the high-frequency backtest remains explicitly skipped upstream. The normal workflows also passed their lint, type, notebook, and pipeline checks; documentation builds passed where enabled.
  • Two normal jobs needed the workflow's automatic second attempt: Ubuntu 24.04/Python 3.10 failed the existing RL early-stop assertion initially, and Windows/Python 3.10 exceeded the existing MLflow client-creation timing threshold. Both then passed. These are final successful results, not an assertion that every first attempt passed.

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

@nkjapan

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@nkjapan
Natsuki Komatsu (nkjapan) marked this pull request as ready for review October 5, 2026 01:50
@arhancanli

Copy link
Copy Markdown

The diagnosis checks out: np.cov uses N-1 and np.var uses N, so for r == b the old ratio is N/(N-1) (1.25 for five points), and taking both terms from one covariance matrix is the right fix.

Three input cases the new tests don't pin down, which I ran against the new expression cov[0, 1] / cov[1, 1] (numpy 2.5.3, pandas Series inputs):

  • A constant benchmark (zero variance), e.g. [0.001] * 5, returns nan with RuntimeWarning: invalid value encountered in scalar divide. That is reasonable, but a test with pytest.warns or an np.isnan assertion would document it, since get_alpha passes it straight through.
  • One NaN in the strategy returns (for instance the first row after a pct_change) makes beta nan, and therefore alpha nan, with no warning. If callers are expected to drop the NaN first, say so in the docstring; if not, pd.concat([r, b], axis=1).dropna() before np.cov would be a small change.
  • Series of different lengths raise ValueError: all the input array dimensions except for the concatenation axis must match exactly... from np.cov rather than aligning on the index. This is unchanged from before, but a docstring line that the two series must be aligned would help, because r and b come from different sources in most backtests.

None of these block the PR; the first is probably worth one assertion, since the fix changes the return type from matrix to scalar and any caller that relied on a nan matrix would now see a scalar nan.

@nkjapan

Copy link
Copy Markdown
Author

The diagnosis checks out: np.cov uses N-1 and np.var uses N, so for r == b the old ratio is N/(N-1) (1.25 for five points), and taking both terms from one covariance matrix is the right fix.

Three input cases the new tests don't pin down, which I ran against the new expression cov[0, 1] / cov[1, 1] (numpy 2.5.3, pandas Series inputs):

  • A constant benchmark (zero variance), e.g. [0.001] * 5, returns nan with RuntimeWarning: invalid value encountered in scalar divide. That is reasonable, but a test with pytest.warns or an np.isnan assertion would document it, since get_alpha passes it straight through.
  • One NaN in the strategy returns (for instance the first row after a pct_change) makes beta nan, and therefore alpha nan, with no warning. If callers are expected to drop the NaN first, say so in the docstring; if not, pd.concat([r, b], axis=1).dropna() before np.cov would be a small change.
  • Series of different lengths raise ValueError: all the input array dimensions except for the concatenation axis must match exactly... from np.cov rather than aligning on the index. This is unchanged from before, but a docstring line that the two series must be aligned would help, because r and b come from different sources in most backtests.

None of these block the PR; the first is probably worth one assertion, since the fix changes the return type from matrix to scalar and any caller that relied on a nan matrix would now see a scalar nan.

@nkjapan

Copy link
Copy Markdown
Author

Thanks for reviewing this and for the suggestions! I’ve added regression tests for the constant-benchmark case and clarified the input assumptions in the docstring, including positional matching and NaN handling. The focused tests pass, and the calculation itself is unchanged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants