QuantEcon.py icon indicating copy to clipboard operation
QuantEcon.py copied to clipboard

Migrate `np.dot` to `@` Operator

Open HumphreyYang opened this issue 2 years ago • 7 comments

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.

HumphreyYang avatar Jan 06 '23 04:01 HumphreyYang

Coverage Status

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

coveralls avatar Jan 06 '23 04:01 coveralls

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

jstac avatar Jan 06 '23 06:01 jstac

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 avatar Jan 09 '23 11:01 HumphreyYang

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.

HumphreyYang avatar Jan 15 '23 12:01 HumphreyYang

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?

mmcky avatar Oct 24 '23 23:10 mmcky

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.

HumphreyYang avatar Oct 25 '23 00:10 HumphreyYang

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 '('

pep8speaks avatar Oct 25 '23 01:10 pep8speaks