MAINT: Migrate np.dot()/.dot() to @ in the test suite - #798
Conversation
Co-authored-by: mmcky <8263752+mmcky@users.noreply.github.com>
mmcky
left a comment
There was a problem hiding this comment.
@HumphreyYang this looks pretty good to me.
From memory there was another pattern we needed to update (is that right?)
Follow-up tidy-up on the lines this PR already touches: - test_kalman.py: fix the continuation-line indents left over from the longer .dot() chains (E127 over-indent on sig_recursion, E128 under-indent on new_sigma), and unwrap new_sigma now that it fits on one line (76 chars). - test_matrix_eqn.py: unwrap the assert_allclose now that the migrated call is 59 chars, and spell the conjugate transpose .conj().T, which is the dominant form in this codebase. - test_lqcontrol.py: document why the scalar term uses * rather than @. lq_scalar.C is (1, 1) and w_seq[0, -1] is 0-d, so @ raises ValueError; np.dot and * both give the same (1, 1) result. flake8 on the seven touched files is now strictly cleaner than main: E127 and E128 are gone and no new diagnostic is introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refreshed and polished — ready for reviewThis had gone stale (11 months, 48 commits on Answering my own open question aboveI asked whether there was another pattern we needed to update. There was, and it is The
Two things that are deliberately not gaps, so they don't get re-raised: the residual Why it needed a push, not just un-draftingBranch protection now requires Tidy-ups, all on lines the PR already touches
Description rewritten as well: dropped the claim that Verification
Still neededLead developer approval before merge. Suggested squash title: One follow-up worth filing separately: |
There was a problem hiding this comment.
Pull request overview
This PR updates the QuantEcon.py test suite to use Python’s matrix-multiplication operator (@, PEP 465) in place of remaining np.dot(...) and .dot(...) usages, aligning test code with the library migration done in #787 and improving readability of chained products.
Changes:
- Replaced
np.dot()/.dot()call sites with@across multiple test modules. - Removed the now-unused
from numpy import dotimport intest_lqcontrol.py. - Minor formatting tidy-ups in touched expressions (line wrapping / indentation).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| quantecon/tests/test_kalman.py | Converts covariance / gain computations from dot chains to @ expressions. |
| quantecon/tests/test_lqnash.py | Replaces matrix products in nnash validation with @. |
| quantecon/tests/test_lqcontrol.py | Drops dot import; uses @ where valid and * for the documented 0-d scalar case. |
| quantecon/tests/test_matrix_eqn.py | Simplifies Lyapunov residual check to A @ X @ A.conj().T. |
| quantecon/tests/test_ricatti.py | Rewrites Riccati test expression using @ (note: one issue flagged in review). |
| quantecon/markov/tests/test_core.py | Uses @ for left-eigenvector checks (vP = v). |
| quantecon/markov/tests/test_gth_solve.py | Uses @ for left eigenvector assertion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@HumphreyYang would have any time to review this? |
Catches the @-operator migration up to main (#831, #937, #947, #949). Conflict in quantecon/tests/test_matrix_eqn.py: #831 appended test_solve_discrete_riccati_system_beta_ge_one_failure_message directly after the assert this branch rewrote to the @ operator. Kept both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merged @HumphreyYang would you be able to give this a final review before merge? |
mmcky
left a comment
There was a problem hiding this comment.
Approving — verified as behaviour-preserving two independent ways
I set out to answer one question: does any hunk here change what the code actually does? It does not. Two checks, deliberately chosen so that neither depends on the other being right.
1. Runtime bitwise equivalence
I instrumented all 18 changed statements in a scratch worktree so that each one computes both the pre-diff form and the post-diff form, then asserted identical type, shape, dtype and bitwise value at every evaluation. The suite passes with those assertions live: 86 tests, 27 evaluations of the instrumented sites, zero non-bitwise results and zero type changes.
The instrumentation also recorded operand metadata, which settles the question that motivated the review: every @ operand is an ndarray with ndim <= 2. That rules out all three known np.dot/@ divergences — no operand has ndim > 2 (where np.dot sum-products while @ broadcasts a stack), none is a Python list, and none is 0-d except the one case handled below. The complex128 path in test_matrix_eqn.py is included, and .conj().transpose() to .conj().T is a no-op for any ndim, since ndarray.T is defined as transpose().
2. AST equivalence, which is the stronger check
Runtime equality only proves the fixtures agree. Because floating-point matmul is not associative, a silent re-association would be a genuine bug that the right test values could hide. So I parsed the pre- and post-diff versions of all seven files, normalised every dot form (np.dot(a, b), bare dot(a, b), a.dot(b)) to a @ b and .transpose() to .T, and compared the full syntax trees.
Six of the seven files are structurally identical after normalisation. That is a proof of preserved grouping and left-associativity for all inputs, not just the ones the suite happens to use. test_lqcontrol.py shows exactly two deviations, both intentional: the removed from numpy import dot, and a single MatMult to Mult at the 0-d site. Nothing anywhere else in these files deviates from a pure mechanical rewrite — and because the comparison covers whole files rather than just the hunks, that also confirms the reindents and unwrappings are purely cosmetic.
The one site where @ was never an option
Confirming the carve-out at test_lqcontrol.py:55-58 independently, since it is the only non-mechanical hunk:
| form | result |
|---|---|
np.dot(C, w) |
ndarray (1, 1) float64 |
C * w |
ndarray (1, 1) float64, bitwise identical |
C @ w |
ValueError: matmul: Input operand 1 does not have enough dimensions |
LQ puts C through np.atleast_2d so lq_scalar.C is (1, 1), while w_seq[0, -1] is a 0-d np.float64. * is correct, matches the two sibling terms in the same statement, and the explanatory comment earns its place. Consistent with the _lqnash.py carve-out from #787, which I confirmed is still intact on this branch.
Claims spot-checked
All hold: 38 call sites across 18 statements (counted independently); zero .dot( or bare dot( remaining anywhere under the test tree; flake8 strictly cleaner on the touched files, with E127 and E128 gone and no new diagnostic; the --select=F401,F405,E231 gate exits 0. The full suite passes 678, and the collected test-ID set is identical to main — a stricter check than the count alone, and the one that actually rules out an import-order side effect from dropping the module-level dot import.
The Copilot objection on test_ricatti.py:65 does not hold, and your rebuttal is right on all three counts. list.__add__ returns NotImplemented for an ndarray, so ndarray.__radd__ coerces and R + I evaluates to [[2.0, 0.5], [0.5, 1.25]]. A is also np.array(A) on the line above, and the sub-expression is byte-identical before and after.
I agree with leaving np.sum() alone, and with Refs #790 rather than a closing reference.
One follow-up, filed separately
While reading test_left_eigen_vec I noticed its else branch never executes — the KMR fixture is irreducible, so n_stat_dists is always 1. Pre-existing and unrelated to this PR, which rewrites that branch faithfully. Raised as #961 so it does not ride along here.
Review was AI-assisted (Claude Opus 5); every claim above was verified by execution against the merged branch, not asserted from reading.
|
@HumphreyYang I am going to merge this. I am pretty confident in the PR now. |
Test-suite follow-up to #787, which migrated the library code. This replaces the remaining
np.dot()function calls and.dot()method calls in the test suite with Python's@operator (PEP 465).To be clear about the motivation:
np.dotis not deprecated. The reason to move is readability —@reads left-to-right for chained products and removes the nesting that a form likenp.dot(np.dot(A, X), A.conj().T)forces on the reader.Scope
38 call sites across 18 statements in 7 test modules, plus the now-unused
from numpy import dotimport intest_lqcontrol.py.quantecon/tests/test_kalman.pyquantecon/tests/test_lqnash.pyquantecon/tests/test_lqcontrol.pyquantecon/markov/tests/test_core.pyquantecon/tests/test_matrix_eqn.pyquantecon/tests/test_ricatti.pyquantecon/markov/tests/test_gth_solve.pyAfter this lands there are zero
np.dot(/.dot(call sites anywhere underquantecon/**/tests/**.Examples
One deliberate exception
test_lqcontrol.py::test_scalar_sequencesuses*, not@:LQputsCthroughnp.atleast_2d, solq_scalar.Cisarray([[0.05]])whilew_seq[0, -1]is a 0-dnp.float64.@rejects 0-d operands —ValueError: matmul: Input operand 1 does not have enough dimensions— whereasnp.dotand*both return the same(1, 1)float64 result.*also matches the two sibling terms in the same statement, which are alreadylq_scalar.A * x0andlq_scalar.B * u_0. The comment is there so this does not get "fixed" to@by a later reader, since it is the only non-mechanical hunk in the diff.This mirrors the same care taken in #787 for
_lqnash.py, whereS1/S2/W1/W2/M1/M2can be scalar0and the.dot()calls were deliberately left in place.Correctness
Every operand in every changed expression is a 1-D or 2-D
ndarray, so none of the knownnp.dot/@divergences apply: no operand hasndim > 2(wherenp.dotsum-products over the last two axes while@broadcasts as a stack of matrices), none is a Python list, and none is 0-d apart from the case above. Operator precedence preserves the original grouping throughout —@binds tighter than+and-— and where.dot()'s call syntax was supplying implicit parentheses, they were made explicit:Each old/new pair was checked against operands reconstructed from the surrounding fixtures, and agrees bitwise in shape, dtype and value.
One incidental gain worth noting:
MarkovChain.Pmay be asparse.csr_matrix, andnp.dotdoes not dispatch to sparse operands (it returns adtype=objectarray, and the followingassert_allclosethen fails confusingly), whereassd @ csrcorrectly reaches__rmatmul__. Behaviour is unchanged today because those fixtures are dense, but the@form is the one that would survive parametrisingtest_left_eigen_vecover sparseP.Tidy-ups on lines already touched
test_kalman.py— fixed the continuation-line indents left behind once the expressions got shorter (E127 over-indent onsig_recursion, E128 under-indent onnew_sigma), and unwrappednew_sigma, which now fits on one line at 76 characters.test_matrix_eqn.py— unwrapped theassert_allclose, now 59 characters, and spelled the conjugate transpose.conj().T, which is the dominant form in this codebase (172.Tagainst 3.transpose()).flake8on the seven touched files is now strictly cleaner thanmain: E127 and E128 are gone, and no new diagnostic is introduced. The CI gate,flake8 --select=F401,F405,E231 quantecon, exits 0.Testing
605 passed locally on Python 3.13.9 / NumPy 2.3.5 / SciPy 1.16.3 / numba 0.62.1 — the same count as
main, and the same set of collected tests, which rules out an import-order side effect from dropping the module-leveldotimport.Relationship to #790
Refs #790. That issue also names
np.sum(), which this PR deliberately leaves alone, so it should not close the issue outright.np.sumis not deprecated and has no PEP 465-style replacement, sonp.sum(x)versusx.sum()is cosmetic — andnp.sum()is in any case already the dominant form here, 20 uses against 2.sum()method calls across the package, so migrating would convert the majority form into the minority one. #787 also left all seven librarynp.sumcall sites standing, so there is no agreed target form to migrate towards. 13np.sum(calls across 5 test files are untouched by this PR.