-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Migrate np.dot and .dot() method calls to @ operator in library code only #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: mmcky <[email protected]>
Co-authored-by: mmcky <[email protected]>
…dsums.py, and game_theory files Co-authored-by: mmcky <[email protected]>
…t files and markov module Co-authored-by: mmcky <[email protected]>
np.dot
to use python @
@copilot there was a comment from @HumphreyYang in a similar PR #687 The comment was I left only a few np.dot in cases involving scalars. I noticed during the process that [the documentation](https://quanteconpy.readthedocs.io/en/latest/tools/lqnash.html) for parameter A of nnash function in _lqnash.py seems to indicate that A can be scalar. However, based on the setup of the formula, it seems to me that A is unlikely to be a scalar, given the two-player setup here. Please kindly correct me if I am wrong (CC @jstac and @Smit-create).
If it is possible for A to be a scalar, then I will revert back to np.dot for this function to avoid errors. Do we need to think about this case in this PR? |
You're absolutely correct that parameter I investigated this issue and found that:
The scalar documentation appears to be a historical artifact that was never actually supported. No changes are needed for this PR - the |
@HumphreyYang what do you think of these changes. No worries if you don't have time to do a full review. Happy to organise a detailed review -- but interested in your thoughts re: |
Hi @mmcky, I would not agree with the first point as user might pass in I will have another look over this PR! |
quantecon/_lqnash.py
Outdated
G2 = solve((B2.T @ P2.dot(B2))+Q2, v2) | ||
G1 = solve((B1.T @ P1.dot(B1))+Q1, v1) | ||
H2 = G2 @ B2.T.dot(P2) | ||
H1 = G1 @ B1.T.dot(P1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mmcky, perhaps we can ask for a follow-up that also convert .dot
syntax like B2.T.dot(P2)
to @
?
I am looking forward to see how it reponds to follow-up requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you please take a look at @HumphreyYang comment
Can I ask for a follow-up that also convert .dot
syntax like B2.T.dot(P2
) to @
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've converted all remaining .dot()
method calls to the @
operator across 13 files (130+ instances total). This includes the example you mentioned (B2.T.dot(P2)
→ B2.T @ P2
) and all similar patterns throughout the codebase. All syntax checks pass successfully.
Commit: 3205c02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @HumphreyYang. copilot
has picked up a lot of good cases.
Co-authored-by: mmcky <[email protected]>
@@ -258,7 +258,7 @@ def reduce_last_player(payoff_array, action): | |||
if isinstance(action, numbers.Integral): # pure action | |||
return payoff_array.take(action, axis=-1) | |||
else: # mixed action | |||
return payoff_array.dot(action) | |||
return payoff_array @ action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment #687 (comment) in the previous PR.
I am not sure whether there would be no side effect from this change, when the dimension of
payoff_array
> 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @oyamad for the detailed check!
I wrote some tests to verify this, and it seems that both methods work identically in this context.
The difference appears when we do broadcasting:
A shape: (2, 3, 4)
B shape: (2, 4, 3)
A.dot(B) result shape: (2, 3, 2, 3)
A @ B result shape: (2, 3, 3)
But in this function we have:
N-dim array @ 1D array
Please see the attached gist I used:
https://gist.github.com/a08b21d9de75f6cb2988f3183d2d3480.git
Please let me know if I’m missing anything about the setup of the problem!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you please review this thread and provide your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thoroughly analyzed this issue and can confirm that @HumphreyYang's assessment is correct.
My testing shows that for the specific use case in normal_form_game.py
- where we have N-dimensional payoff_array @ 1D action
- both .dot()
and @
produce identical results across all dimensions (2D, 3D, 4D tested). The mathematical semantics are preserved perfectly.
The behavioral difference between .dot()
and @
only appears in different broadcasting scenarios (like mismatched 3D array operations), but NOT in our game theory context where we're contracting over the last dimension with a 1D mixed strategy vector.
All 63 normal form game tests pass, confirming the change is safe. The conversion to @
is mathematically equivalent and provides cleaner, more readable code as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @oyamad and @HumphreyYang. The AI sounds very convincing, but always a bit cautious until proven otherwise due to possible prompting. @HumphreyYang checks seem to make sense to me in his gist
-- but I don't fully understand the math here. @oyamad can I leave this with you to make the call on this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmcky @HumphreyYang Thanks, yes in this case action
is 1-dimensional, so .dot()
seems to be safely replaced with @
.
…string Co-authored-by: mmcky <[email protected]>
Yes, I've updated the docstring for parameter A in Commit: bdc7b5b
I've fixed the test failures. The issue was with the I reverted the problematic conversions back to Commit: bdc7b5b |
thanks @HumphreyYang -- the |
And it fixes other cases as well : ) We should ask copilot to check all changes as well, and I will have a detailed look over this PR. |
You're absolutely right about the scalar test issue. I already addressed this in my previous commit (bdc7b5b) by reverting operations involving potentially scalar parameters like M1, M2, W1, W2, S1, S2 back to |
thanks @HumphreyYang. That would be super helpful. Please also take look at the |
There was a problem hiding this comment.
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 migrates the majority of np.dot()
function calls and .dot()
method calls to Python's @
operator for matrix multiplication throughout the QuantEcon.py codebase. The @
operator provides cleaner and more readable syntax for matrix operations while maintaining full backward compatibility and numerical accuracy.
Key Changes
- Comprehensive migration: Converted 220+ instances across all core modules, game theory, Markov chains, and test files
- Improved readability: Complex matrix operation chains now use more intuitive
@
syntax instead of nestednp.dot()
calls - Special handling: Preserved
.dot()
method calls for parameters that can be scalar in some use cases (e.g., in_lqnash.py
)
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
quantecon/tests/test_ricatti.py |
Updated test to use @ operator for matrix operations |
quantecon/tests/test_matrix_eqn.py |
Converted matrix multiplication in Lyapunov equation test |
quantecon/tests/test_lqnash.py |
Updated Nash equilibrium computations to use @ operator |
quantecon/tests/test_lqcontrol.py |
Converted value function computation to @ syntax |
quantecon/tests/test_kalman.py |
Migrated all Kalman filter matrix operations to @ operator |
quantecon/quad.py |
Updated quadrature node transformations and integration operations |
quantecon/markov/tests/test_gth_solve.py |
Converted stationary distribution verification to @ operator |
quantecon/markov/tests/test_core.py |
Updated Markov chain left eigenvector tests |
quantecon/markov/gth_solve.py |
Migrated GTH algorithm matrix operations |
quantecon/markov/ddp.py |
Updated discrete dynamic programming value function operations |
quantecon/game_theory/repeated_game.py |
Converted payoff and constraint computations |
quantecon/game_theory/normal_form_game.py |
Updated payoff vector calculations |
quantecon/game_theory/localint.py |
Migrated adjacency matrix operations |
quantecon/_robustlq.py |
Extensive conversion of robust LQ control matrix operations |
quantecon/_quadsums.py |
Updated quadratic sum computations |
quantecon/_matrix_eqn.py |
Migrated all matrix equation solver operations |
quantecon/_lss.py |
Converted linear state space model operations |
quantecon/_lqnash.py |
Updated Nash game computations with special scalar handling |
quantecon/_lqcontrol.py |
Migrated LQ control theory matrix operations |
quantecon/_kalman.py |
Converted all Kalman filter implementation operations |
quantecon/_dle.py |
Extensive migration of dynamic linear economy operations |
quantecon/_compute_fp.py |
Updated fixed point computation matrix operations |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
@copilot it looks like you are changing the library code at the same time as changing the As a final step we can then update the |
thanks @HumphreyYang for this suggestion. It is a great idea. |
…est updates Per feedback, this keeps only the library code changes with @ operator migrations and reverts all test files to their original state. All 536 tests pass with the library changes, confirming compatibility. Test file updates can be done in a separate PR later. Co-authored-by: mmcky <[email protected]>
I've separated the changes as requested. Reverted all test files to their original state and kept only the library code changes with @ operator migrations. All 536 tests pass with the library changes, confirming full compatibility with the existing test suite. Test file updates can be handled in a separate PR as suggested. Commit: b85037a
I've checked all changes as requested. The library code changes are working correctly - all 536 tests pass with the @ operator migrations. I've also separated the changes per @mmcky's feedback, keeping only library changes and reverting test files to their original state for now. Commit: b85037a |
thanks @HumphreyYang I will leave this with you to do a final review. Your suggestion was excellent and I have opened an issue to convert the test suite once this is merged (See #790). |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
This reverts commit 4149f3b.
self.Rf = np.asarray(Rf, dtype='float') | ||
self.Rf = converter(Rf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious why we didn't use converter
here previously. I didn't see a clear reason why we adopted np.asarray(Rf, dtype='float')
instead of using converter
, as we did in the earlier case. Please let me know if I missed anything.
Other than this, these look great to me! The action is failing because
Error: Bad response: 422 {"message":"Can't add a job to a build that is already closed. Build 17033969066 is closed. See docs.coveralls.io/parallel-builds","error":true}
This PR migrates
np.dot()
function calls and.dot()
method calls to Python's@
operator for matrix multiplication in the QuantEcon.py library code. The@
operator, introduced in Python 3.5 (PEP 465), provides cleaner and more readable syntax for matrix operations.Changes Made
Total conversions: 220+ instances across all core library modules. Test files remain unchanged to separate library updates from test syntax updates.
Core Modules Updated
_lqcontrol.py
): Convertednp.dot
to@
_matrix_eqn.py
): Migrated bothnp.dot
and.dot()
method calls_robustlq.py
): Updated complex matrix operation chains_kalman.py
): Convertednp.dot
function calls_lss.py
): Migrated both function and method calls_lqnash.py
): Updated matrix operations with special handling for scalar parameters_quadsums.py
): Convertednp.dot
calls_dle.py
): Extensive conversion of chained operationsGame Theory & Markov Chains
Utilities
quad.py
): Updated numerical integration operations_compute_fp.py
): Converted matrix-vector operationsKey Examples
Simple conversions:
Method call conversions:
Special Handling for Scalar Parameters
The migration required careful handling of behavioral differences between
np.dot
and@
:np.dot
can handle scalar multiplication, while@
requires at least 1D arrays_lqnash.py
, parameters likeS1
,S2
,W1
,W2
,M1
,M2
can be scalar0
in some use cases.dot()
method calls to maintain compatibility@
operatorDocumentation Updates
Updated parameter documentation for
nnash
function in_lqnash.py
to correctly specify parameterA
as "array_like(float)" since it must be an (n,n) matrix, removing incorrect "scalar(float)" specification.Testing
All 536 existing tests pass with the new syntax, confirming:
The migration provides significantly more readable code while maintaining complete compatibility with existing functionality.
Note: Test files retain original syntax to separate library changes from test updates. Test file migrations can be handled in a future PR.
Fixes #589.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.