Skip to content

MAINT: Migrate np.dot()/.dot() to @ in the test suite - #798

Open
mmcky with Copilot wants to merge 5 commits into
mainfrom
copilot/fix-790
Open

MAINT: Migrate np.dot()/.dot() to @ in the test suite#798
mmcky with Copilot wants to merge 5 commits into
mainfrom
copilot/fix-790

Conversation

Copilot AI commented Sep 2, 2025

Copy link
Copy Markdown
Contributor

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.dot is not deprecated. The reason to move is readability — @ reads left-to-right for chained products and removes the nesting that a form like np.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 dot import in test_lqcontrol.py.

File Call sites Statements
quantecon/tests/test_kalman.py 25 9
quantecon/tests/test_lqnash.py 3 2
quantecon/tests/test_lqcontrol.py 3 2
quantecon/markov/tests/test_core.py 2 2
quantecon/tests/test_matrix_eqn.py 2 1
quantecon/tests/test_ricatti.py 2 1
quantecon/markov/tests/test_gth_solve.py 1 1

After this lands there are zero np.dot( / .dot( call sites anywhere under quantecon/**/tests/**.

Examples

# before
Q = I - np.dot(A.T, A) + np.dot(A.T, np.linalg.solve(R + I, A))
sig_recursion = (A.dot(sig_inf).dot(A.T) -
                    kal_recursion.dot(G).dot(sig_inf).dot(A.T) + Q)
val_func_lq = np.dot(x0, P).dot(x0)

# after
Q = I - A.T @ A + A.T @ np.linalg.solve(R + I, A)
sig_recursion = (A @ sig_inf @ A.T -
                 kal_recursion @ G @ sig_inf @ A.T + Q)
val_func_lq = x0 @ P @ x0

One deliberate exception

test_lqcontrol.py::test_scalar_sequences uses *, not @:

# C is (1, 1) and w_seq[0, -1] is 0-d, so this term is * not @:
# @ rejects 0-d operands, while np.dot (and *) multiply.
x_1 = lq_scalar.A * x0 + lq_scalar.B * u_0 + \
    lq_scalar.C * w_seq[0, -1]

LQ puts C through np.atleast_2d, so lq_scalar.C is array([[0.05]]) while w_seq[0, -1] is a 0-d np.float64. @ rejects 0-d operands — ValueError: matmul: Input operand 1 does not have enough dimensions — whereas np.dot and * both return the same (1, 1) float64 result. * also matches the two sibling terms in the same statement, which are already lq_scalar.A * x0 and lq_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, where S1/S2/W1/W2/M1/M2 can be scalar 0 and 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 known np.dot/@ divergences apply: no operand has ndim > 2 (where np.dot sum-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:

# before
new_xhat = A.dot(curr_x) + curr_k.dot(y_observed - G.dot(curr_x))
# after
new_xhat = A @ curr_x + curr_k @ (y_observed - G @ curr_x)

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.P may be a sparse.csr_matrix, and np.dot does not dispatch to sparse operands (it returns a dtype=object array, and the following assert_allclose then fails confusingly), whereas sd @ csr correctly reaches __rmatmul__. Behaviour is unchanged today because those fixtures are dense, but the @ form is the one that would survive parametrising test_left_eigen_vec over sparse P.

Tidy-ups on lines already touched

  • test_kalman.py — fixed the continuation-line indents left behind once the expressions got shorter (E127 over-indent on sig_recursion, E128 under-indent on new_sigma), and unwrapped new_sigma, which now fits on one line at 76 characters.
  • test_matrix_eqn.py — unwrapped the assert_allclose, now 59 characters, and spelled the conjugate transpose .conj().T, which is the dominant form in this codebase (172 .T against 3 .transpose()).

flake8 on the seven touched files is now strictly cleaner than main: 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-level dot import.

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.sum is not deprecated and has no PEP 465-style replacement, so np.sum(x) versus x.sum() is cosmetic — and np.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 library np.sum call sites standing, so there is no agreed target form to migrate towards. 13 np.sum( calls across 5 test files are untouched by this PR.

Co-authored-by: mmcky <8263752+mmcky@users.noreply.github.com>
Copilot AI changed the title [WIP] Migrate np.sum(), np.dot and some .dot() methods in test suite Migrate np.dot(), .dot() methods in test suite to @ operator Sep 2, 2025
Copilot AI requested a review from mmcky September 2, 2025 02:38
@coveralls

coveralls commented Sep 15, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 90.57%. remained the same — copilot/fix-790 into main

Comment thread quantecon/tests/test_lqcontrol.py

@mmcky mmcky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@HumphreyYang this looks pretty good to me.

From memory there was another pattern we needed to update (is that right?)

mmcky and others added 2 commits August 14, 2026 14:17
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>
@mmcky mmcky changed the title Migrate np.dot(), .dot() methods in test suite to @ operator MAINT: Migrate np.dot()/.dot() to @ in the test suite Aug 14, 2026
@mmcky
mmcky marked this pull request as ready for review August 14, 2026 04:30
Copilot AI lite review requested due to automatic review settings August 14, 2026 04:30
@mmcky

mmcky commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Refreshed and polished — ready for review

This had gone stale (11 months, 48 commits on main), so I've brought it up to date, tidied the diff, and rewritten the description. Summary of what changed and why.

Answering my own open question above

I asked whether there was another pattern we needed to update. There was, and it is np.sum() — from my comment on #787: "we can then update the test solutions with the new syntax replacing np.sum(), np.dot() and most .dot() patterns ... perhaps in a second PR." This PR is that second PR, and it covers two of those three.

The np.dot / .dot() side is now exhaustively done: after this merges there are zero np.dot( / .dot( call sites anywhere under quantecon/**/tests/**. The 48 commits since September introduced no new ones either.

np.sum() is untouched — 13 calls across 5 test files. My recommendation is that we accept that and not do it. np.sum is not deprecated and has no PEP 465-style replacement, so np.sum(x) versus x.sum() is purely cosmetic. More to the point, np.sum() is already the dominant form here — 20 uses against 2 .sum() method calls across the whole package — so a migration would convert the majority form into the minority one. #787 also left all seven library np.sum call sites standing, so there is no agreed target form to migrate towards. On that basis I've changed Fixes #790 to Refs #790 so merging doesn't silently close an issue with a third of its title untouched — I'll retitle #790 separately.

Two things that are deliberately not gaps, so they don't get re-raised: the residual .dot() calls in _lqnash.py are intentional (S1/S2/W1/W2/M1/M2 can be scalar 0, where @ raises and .dot() works — this was carved out in #787), and _lss.py:54 is a commented-out line.

Why it needed a push, not just un-drafting

Branch protection now requires tests (ubuntu-latest, 3.12 / 3.13 / 3.14), tests (windows-latest, 3.14) and tests (macos-latest, 3.14). This branch predated #820 ("CI: Add Python 3.14, drop 3.11"), so its runs only ever produced 3.11/3.12/3.13 job names. Both ci.yml and ci_np2.yml declare on: pull_request: with no types: key, so ready_for_review fires nothing, and re-running the old workflow just replays the old matrix. Merging main in was the only way to make the required contexts exist. The merge was clean — no conflicts, despite 48 intervening commits.

Tidy-ups, all on lines the PR already touches

File Change
test_kalman.py Fixed the continuation-line indents left behind once the expressions got shorter (E127 over-indent on sig_recursion, E128 under-indent on new_sigma), and unwrapped new_sigma, which now fits on one line at 76 chars
test_matrix_eqn.py Unwrapped the assert_allclose, now 59 chars, and spelled the conjugate transpose .conj().T — the dominant form here, 172 .T against 3 .transpose()
test_lqcontrol.py Added a source comment recording why the scalar term uses * rather than @ (see the thread above)

Description rewritten as well: dropped the claim that np.dot is "deprecated" (it isn't — the motivation is PEP 465 readability, as argued in #787), replaced the vague "13 migration patterns" with the actual count of 38 call sites across 18 statements in 7 modules, and refreshed the stale "553 tests".

Verification

  • 605 passed locally on Python 3.13.9 / NumPy 2.3.5 / SciPy 1.16.3 / numba 0.62.1, and 605 passed on Python 3.14.7 / NumPy 2.5.2 / SciPy 1.18.0 / numba 0.67.0 — same count and same collected set as main, which rules out an import-order side effect from dropping the module-level from numpy import dot.
  • Every old/new expression pair checked against operands reconstructed from the surrounding fixtures: bitwise identical in shape, dtype and value. No operand has ndim > 2, none is a Python list, and none is 0-d apart from the documented * case — so none of the np.dot/@ divergences apply.
  • flake8 on the seven touched files is now strictly cleaner than main: E127 and E128 are gone and no new diagnostic appears. The CI gate flake8 --select=F401,F405,E231 quantecon exits 0.
  • No library, workflow or config file is touched — the diff is tests-only.

Still needed

Lead developer approval before merge. Suggested squash title: MAINT: Migrate np.dot()/.dot() to @ in the test suite (#798), dropping the Initial plan and Merge branch 'main' bullets from the generated body.

One follow-up worth filing separately: .github/copilot-instructions.md still says "536 tests" and "Python 3.11, 3.12, 3.13", which is exactly the CI mismatch that stalled this PR — and it's the zero-cost place to record the @ convention along with its scalar-operand exception.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 dot import in test_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.

Comment thread quantecon/tests/test_ricatti.py
@mmcky
mmcky requested a review from HumphreyYang August 14, 2026 05:09
@mmcky

mmcky commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

@HumphreyYang would have any time to review this?

@mmcky mmcky added the review label Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants