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

Adding test_randomseed to TestLQNash #717

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

Conversation

hsalberti
Copy link

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

@pep8speaks
Copy link

pep8speaks commented Oct 23, 2023

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

@jstac
Copy link
Contributor

jstac commented Oct 23, 2023

Thanks @hsalberti , much appreciated.

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

@mmcky
Copy link
Contributor

mmcky commented Oct 23, 2023

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

@mmcky
Copy link
Contributor

mmcky commented Oct 24, 2023

@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) =============

@oyamad
Copy link
Member

oyamad commented Oct 25, 2023

Looking at the code, randomness at

F1 = random_state.standard_normal((k_1, n))
F2 = random_state.standard_normal((k_2, n))
is almost irrelevant. The new F1 and F2 are deterministically redefined here
F1 = solve(F1_left, F1_right)
F2 = H2.dot(A)+G2.dot(W2.T) - np.dot(H2.dot(B1)+G2.dot(M2.T), F1)
and the old (random) F1 and F2 are used (as F10 and F20) only for the convergence check here
dd = np.max(np.abs(F10 - F1)) + np.max(np.abs(F20 - F2))

Replacing these

F1 = random_state.standard_normal((k_1, n))
F2 = random_state.standard_normal((k_2, n))
by deterministic arrays with some large value will suffice.

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

@hsalberti
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

5 participants