Repository navigation
BUG: Promote discrete covariance inputs before products - #942
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
bashtage
left a comment
There was a problem hiding this comment.
I think only integer types should be cast to float.
| CovarianceEstimate | ||
| """ | ||
| x = np.ascontiguousarray(self._x) | ||
| x = np.ascontiguousarray(self._x, dtype=float) |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8bad2df to
c907315
Compare
| 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) |
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.
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.
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.
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
Volumedata 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.