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? |
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.