Skip to content
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

Migrate np.dot to @ Operator #687

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

HumphreyYang
Copy link
Collaborator

@HumphreyYang HumphreyYang commented Jan 6, 2023

This PR resolves #589 as it completed the migration to @.

I left only a few np.dot in cases involving scalars. I noticed during the process that the documentation 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.

Many thanks.

@coveralls
Copy link

coveralls commented Jan 6, 2023

Coverage Status

Coverage: 92.779% (-0.07%) from 92.844% when pulling 384e36a on HumphreyYang:change-dot into 65ac63d on QuantEcon:main.

@HumphreyYang HumphreyYang marked this pull request as ready for review January 6, 2023 04:34
@jstac
Copy link
Contributor

jstac commented Jan 6, 2023

Thanks @HumphreyYang , much appreciated. Perhaps @mmcky will review and merge when he finishes his break.

Comment on lines 127 to 129
F1_left = v1 - (H1 @ B2 + G1.dot(M1.T)) @ (H2 @ B1 + G2.dot(M2.T))
F1_right = H1 @ A + G1.dot(W1.T) - (H1 @ B2 + G1.dot(M1.T)) @ \
(H2 @ A + G2.dot(W2.T))
Copy link
Member

Choose a reason for hiding this comment

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

Here are some .dot. Are these kept intentionally?

Copy link
Collaborator Author

@HumphreyYang HumphreyYang Jan 6, 2023

Choose a reason for hiding this comment

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

I left only a few np.dot in cases involving scalars. I noticed during the process that the documentation 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.

Many thanks @Smit-create. Yes, as noted before, these are cases where scalars appears in np.dot, and @ is not capable of multiplying matrices with scalars.

@HumphreyYang
Copy link
Collaborator Author

Hi @mmcky,

Could you please kindly review this PR when you have time? Sorry for mentioning you directly since I haven't been granted access that allows me to request reviews in this repo.

@HumphreyYang
Copy link
Collaborator Author

HumphreyYang commented Jan 15, 2023

I left only a few np.dot in cases involving scalars. I noticed during the process that the documentation for parameter A of nnash function in _lqnash.py seems to indicate that A can be scalar.

Hi @oyamad,

Here is a short update on this PR. I updated the scalar terms to at least 2d to match the matrices in _lqcontrol.py so that np.dot can be transformed into @. I believe the same can be done for _lqnash.py, but before that, confirmations on the input restrictions of nnash function will be very useful as a guide.

Many thanks in advance.

@mmcky
Copy link
Contributor

mmcky commented Oct 24, 2023

thanks @HumphreyYang -- sorry this has taken me a long time.

I think this change looks good.

My only thoughts are that it is unfortunate to have to mix .dot and @ due to the scalar issues.

Just checking when you say scalar do you mean multiply every element in a matrix by a scalar?

Would 3 * A work for the scalar multiplication case?

@HumphreyYang
Copy link
Collaborator Author

Hi @mmcky,

Many thanks for your suggestion!

Just checking when you say scalar do you mean multiply every element in a matrix by a scalar?

The tricky case I am referring to is where a variable can be both scalar or array. In that case, * will not capture the array case. For example, in lqnash:

    W1 : scalar(float) or array_like(float)
        As above, size (n, k_1)
    W2 : scalar(float) or array_like(float)
        As above, size (n, k_2)
    M1 : scalar(float) or array_like(float)
        As above, size (k_2, k_1)
    M2 : scalar(float) or array_like(float)
        As above, size (k_1, k_2)

But I found a way to enforce at least 2d and overcome this issue (i.e. if it is a scalar 1, then we put it into [[1]]).

I will implement this first and ping you to see if it looks good to you.

Many thanks in advance.

@pep8speaks
Copy link

Hello @HumphreyYang! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 82:57: W291 trailing whitespace
Line 103:80: E501 line too long (88 > 79 characters)
Line 104:13: E128 continuation line under-indented for visual indent
Line 104:80: E501 line too long (80 > 79 characters)
Line 105:71: W291 trailing whitespace
Line 106:33: E128 continuation line under-indented for visual indent
Line 135:37: E127 continuation line over-indented for visual indent
Line 137:37: E127 continuation line over-indented for visual indent
Line 151:80: E501 line too long (83 > 79 characters)
Line 154:13: E128 continuation line under-indented for visual indent
Line 154:80: E501 line too long (82 > 79 characters)
Line 215:80: E501 line too long (85 > 79 characters)
Line 217:80: E501 line too long (88 > 79 characters)
Line 219:80: E501 line too long (90 > 79 characters)
Line 244:21: E128 continuation line under-indented for visual indent
Line 247:80: E501 line too long (104 > 79 characters)
Line 279:42: W291 trailing whitespace
Line 280:17: E128 continuation line under-indented for visual indent
Line 281:42: W291 trailing whitespace
Line 282:17: E128 continuation line under-indented for visual indent
Line 283:42: W291 trailing whitespace
Line 284:17: E128 continuation line under-indented for visual indent
Line 285:42: W291 trailing whitespace
Line 286:17: E128 continuation line under-indented for visual indent
Line 287:42: W291 trailing whitespace
Line 288:17: E128 continuation line under-indented for visual indent
Line 289:42: W291 trailing whitespace
Line 290:17: E128 continuation line under-indented for visual indent
Line 291:42: W291 trailing whitespace
Line 292:17: E128 continuation line under-indented for visual indent
Line 293:42: W291 trailing whitespace
Line 294:17: E128 continuation line under-indented for visual indent
Line 323:53: W291 trailing whitespace
Line 324:13: E128 continuation line under-indented for visual indent
Line 324:80: E501 line too long (92 > 79 characters)

Line 166:39: E225 missing whitespace around operator

Line 187:1: W293 blank line contains whitespace

Line 141:14: E127 continuation line over-indented for visual indent
Line 143:14: E127 continuation line over-indented for visual indent

Line 58:59: W291 trailing whitespace

Line 593:47: E261 at least two spaces before inline comment

Line 47:33: E127 continuation line over-indented for visual indent
Line 80:36: E225 missing whitespace around operator
Line 81:21: E128 continuation line under-indented for visual indent
Line 81:50: E221 multiple spaces before operator

Line 37:21: E201 whitespace after '('

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

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

I would say, unless you are perfectly sure, @ should be restricted to the cases where A in A @ b or A @ B has dimension 2.

I added a comment on one part of the change, but it is just one example.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether there would be no side effect from this change, when the dimension of payoff_array > 2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Many thanks @oyamad,

I think this is a really nice point as the equivalence is only established in 2-D arrays. Although all the tests are passed at the moment, it would be great to be cautious. In cases where N-D.dot(1-D), the behaviour might also be unexpected.

I can write some tests on this while trying edge cases if you think it is a good idea.

Many thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate np.dot to use python @
7 participants