Skip to content

BUG: Promote discrete covariance inputs before products - #942

Merged
bashtage merged 3 commits into
bashtage:mainfrom
antonsoo:fix/integer-covariance-products
Oct 17, 2026
Merged

bashtage merged 3 commits into
bashtage:mainfrom
antonsoo:fix/integer-covariance-products

Conversation

@antonsoo

@antonsoo antonsoo commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Uncentered covariance calculations retain integer or Boolean input dtypes. Products can overflow or use Boolean arithmetic; weighted lag updates can raise casting errors, and the Numba kernel rejects integer matrix products.

import numpy as np
from arch.covariance.kernel import Bartlett
from arch.utility.cov import cov_nw

x = np.array([2**32 + 1, 2**32 + 2, 2**32 + 3], dtype=np.int64)
expected = sum(int(value)**2 for value in x) / len(x)
np.testing.assert_allclose(cov_nw(x, demean=False), expected, rtol=1e-14)
np.testing.assert_allclose(
    Bartlett(x, bandwidth=0, center=False).cov.short_run, expected, rtol=1e-14
)

Promote discrete working covariance arrays to float64 before forming products in both APIs. Preserve existing floating and complex dtypes, caller data and kernel labels.

Regressions compare integer/Boolean lagged products with exact Python-integer and rational-weight references, and check existing float32/complex behavior. Bundled recorded Volume data also agrees with independent lag sums in NumPy and Numba; existing float64 and centered-volume outputs remain identical. PR #672 still reproduces the issue.

Covariance and utility suites, public docstring checks and tracked-source style checks pass. The regressions also pass on Python 3.10 with NumPy 1.22.4 and the older supported numerical dependencies. Scoped Mypy diagnostics match unchanged main.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T23:54:59.911723Z c907315 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@bashtage bashtage left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think only integer types should be cast to float.

Comment thread arch/covariance/kernel.py Outdated
CovarianceEstimate
"""
x = np.ascontiguousarray(self._x)
x = np.ascontiguousarray(self._x, dtype=float)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is too strict. Perhaps they shoudl only be promoted if they are not floating or complex. Something like a second line

dtype=float if np.issubdtype(self._x.dtype, np.integer) else None
x = np.ascontiguousarray(self._x,  dtype=dtype)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated both covariance paths to preserve floating and complex dtypes while still promoting integer and Boolean inputs. The 216 recorded integer cases remain unchanged, and 144 recorded floating/complex controls now match untouched main exactly. Added preservation regressions and rebased onto 49a57a17; the covariance/utility/docstring suite passes (3,483 tests).

@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.95%. Comparing base (49a57a1) to head (c907315).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #942   +/-   ##
=======================================
  Coverage   98.94%   98.95%           
=======================================
  Files         103      104    +1     
  Lines       23766    23829   +63     
  Branches     1901     1909    +8     
=======================================
+ Hits        23516    23579   +63     
  Misses        203      203           
  Partials       47       47           
Flag Coverage Δ
adder 98.88% <100.00%> (+<0.01%) ⬆️
subtractor 98.88% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bashtage added a commit to antonsoo/arch that referenced this pull request Oct 17, 2026
@bashtage
bashtage merged commit d921c35 into bashtage:main Oct 17, 2026
3 of 22 checks passed
Comment thread arch/utility/cov.py
If y is not 1-d or 2-d, is empty or contains NaN or infinite values, or if
lags, axis or ddof are outside their range.
"""
z = asarray(y)
shan3520 pushed a commit to shan3520/arch that referenced this pull request Oct 17, 2026
The kernel covariance estimators, cov_nw and add_trend accept complex data. The
estimators are Hermitian: every product uses the conjugate transpose, so that the lag-j
autocovariance is the sum of x[t] times the conjugate of x[t - j], the long-run
covariance is Gamma_0 + sum of w_j (Gamma_j + Gamma_j^H) and the estimates are
complex128 matrices that are exactly Hermitian, with a real diagonal.

* The automatic bandwidth combines the columns with the (real) weights and uses the real
  part of the autocovariance of the complex series, since the sums over j and -j add a
  value and its conjugate. The product v[t] * v[t - j] of the earlier code is the
  pseudo-covariance, which is not what the bandwidth formula needs.
* cov_nw returns a real float for a 1-d complex series, and a complex Hermitian matrix
  otherwise.
* add_trend adds the real trend columns to complex data and returns complex128. Constant
  columns of complex data are detected.
* PreWhitenedRecolored raises a TypeError for complex data. A VAR for complex data is
  not one model: a VAR with complex coefficients cannot represent real and imaginary
  parts that depend on each other in a way that is not a complex multiplication, the VAR
  of the stacked real and imaginary parts is a different model with more parameters,
  and the order that the information criteria select depends on which is used. The
  Notes say to use the stacked data or a kernel estimator.
* The weights of the bandwidth are real.

The estimates for float32 data are float64, and for complex64 data they are complex128.
The tests of antonsoo's last commit (bashtage#942) expected float32 to be preserved, which the
estimators did not do consistently (short_run was float32 and long_run float64) and the
float32 estimates are not accurate enough, and expected the complex estimates to be the
products without the conjugate. test_preserve_floating_kernel_dtype and
test_preserve_inexact_newey_west are replaced by tests of the same values with float64
and complex128 dtypes and the conjugate transpose.

The reference for the tests is independent of the complex arithmetic. For z = a + ib the
Hermitian long-run covariance of the estimators equals S_aa + S_bb + i (S_ba - S_ab),
where S is the long-run covariance of the real data [a, b] with the same kernel,
bandwidth, centering and degrees of freedom, and the automatic bandwidth equals the one
computed from the sums of the autocovariances of the real and imaginary parts. The
tests cover all ten kernels, every estimate (long run, short run, one sided, one sided
strict), centering and degrees of freedom, a rotation of the data (Hermitian estimates
are invariant to it, the pseudo-covariance is not), real data stored as complex, labels
of a DataFrame, and an exact rational reference for float32 data with a large mean. They
pass with and without numba.
shan3520 pushed a commit to shan3520/arch that referenced this pull request Oct 17, 2026
contributing.rst: rule 5 of the input validation section points to a new "Data types"
section, which has the table of data types by the default and by allow_complex, why
float32 is promoted, why float16 is rejected, where complex data are accepted and why
the others are not, and how to add complex support to a function and to test it against
the real and imaginary parts.

9.0.rst: the entry about the conversion of integer, unsigned integer and Boolean data
states the rules for all data types and credits antonsoo for the first float32 and
complex support of the covariance estimators (bashtage#942, bashtage#943), and a new entry in the new
features describes the Hermitian covariance estimators, add_trend and the bootstraps.
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.

3 participants