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

Adding test_randomseed to TestLQNash

Open hsalberti opened this issue 2 years ago • 6 comments

Additional test to guarantee randomseed is working on lqnash based on issue #349

hsalberti avatar Oct 23 '23 12:10 hsalberti

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

Line 70:17: W291 trailing whitespace Line 71:80: E501 line too long (90 > 79 characters)

Comment last updated at 2023-10-30 11:51:27 UTC

pep8speaks avatar Oct 23 '23 12:10 pep8speaks

Thanks @hsalberti , much appreciated.

@oyamad , could you please review this when you have time?

jstac avatar Oct 23 '23 22:10 jstac

@hsalberti I have approved the CI to run. Thanks for your PR.

mmcky avatar Oct 23 '23 22:10 mmcky

@hsalberti the CI has errored with the following info

=========================== short test summary info ============================
FAILED quantecon/tests/test_lqnash.py::TestLQNash::test_randomseed - assert not True
 +  where True = <function array_equal at 0x7f6779380dc0>(array([[0.67919293, 0.        ]]), array([[0.67919293, 0.        ]]))
 +    where <function array_equal at 0x7f6779380dc0> = np.array_equal
============ 1 failed, 513 passed, 4 warnings in 264.67s (0:04:24) =============

mmcky avatar Oct 24 '23 03:10 mmcky

Looking at the code, randomness at https://github.com/QuantEcon/QuantEcon.py/blob/d1e4d22c873d6e84bb196e5600f555916cc3b86f/quantecon/_lqnash.py#L113-L114 is almost irrelevant. The new F1 and F2 are deterministically redefined here https://github.com/QuantEcon/QuantEcon.py/blob/d1e4d22c873d6e84bb196e5600f555916cc3b86f/quantecon/_lqnash.py#L131-L132 and the old (random) F1 and F2 are used (as F10 and F20) only for the convergence check here https://github.com/QuantEcon/QuantEcon.py/blob/d1e4d22c873d6e84bb196e5600f555916cc3b86f/quantecon/_lqnash.py#L144

Replacing these https://github.com/QuantEcon/QuantEcon.py/blob/d1e4d22c873d6e84bb196e5600f555916cc3b86f/quantecon/_lqnash.py#L113-L114 by deterministic arrays with some large value will suffice.

So randomness in this function should be removed in the first place...

oyamad avatar Oct 25 '23 01:10 oyamad

Okay, got it. May I remove "randomstate" from nnash and its documentation?

hsalberti avatar Oct 25 '23 11:10 hsalberti