-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[FIX] Fix Future Warnings in ivp.py and test_quad.py and RuntimeError in lq_control.py. #509
Conversation
thanks @duncanhobbs -- this will be helpful. I will review this in the next couple of days. |
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 @duncanhobbs
In lq_control.py
, beta = 1
should be rejected if C != 0
in this branch
QuantEcon.py/quantecon/lqcontrol.py
Lines 143 to 146 in d9c1e8a
else: | |
self.P = None | |
self.d = None | |
self.T = None |
if (C != 0).any() and beta >= 1:
raise ValueError('beta must be strictly smaller than 1')
Thanks @oyamad. I adjusted the code to incorporate what you suggested, and am going through the other files that use LQ to make sure the other tests do not break. In test_lqnash.py here it raises I think this is because Should the Or only if |
@duncanhobbs Sorry my code was wrong. Try this: if (self.C != 0).any() and beta >= 1:
raise ValueError('beta must be strictly smaller than 1' +
'when T is None and C != 0') |
The problem is in this line:
self.lq_test = LQ(Q, R, A, B, C, beta=beta) (See also QuantEcon/lecture-source-py#314) |
0aff0bb
to
21738ad
Compare
Hello @duncanhobbs! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-09-19 01:40:35 UTC |
21738ad
to
2fa31e2
Compare
Hi @oyamad I have made the changes you requested. I am not sure why the other commits got added from July 11. It may have happened when I was trying to rebase and clean up the commit history. I forgot to rebase my branch before pushing commits to the pull request. Would the easiest way to fix this be to rebase the pull request commits or open a new PR with the feature branch updated? It feels to me that opening a new PR is easier, but I am not sure what standard practice is. Apologies. |
quantecon/tests/test_robustlq.py
Outdated
self.rblq_test_pf = RBLQ(Q_pf, R, A, B_pf, C, beta, theta) | ||
self.lq_test = LQ(Q, R, A, B, C, beta) | ||
self.rblq_test = RBLQ(Q, R, A, B, C, beta=beta, theta=theta) | ||
self.rblq_test_pf = RBLQ(Q_pf, R, A, B_pf, C, beta=beta, theta=theta) |
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.
Are the changes in these two lines really necessary?
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 see they are not. I will fix this.
Thanks @duncanhobbs |
572f49b
to
ae592db
Compare
Thanks @oyamad. I rebased and force-pushed as you suggested and things seem to work. The tests all pass locally. Thanks for your patience. I am not very experienced with using git yet. Let me know if I need to change anything else. |
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.
@duncanhobbs Thank you for your contribution. Much appreciated!
Thanks @duncanhobbs, much appreciated. |
thanks @duncanhobbs and @oyamad for your reviews and advice on this. |
This PR fixes some warnings raised in issues #505 #506 and #507.
FutureWarning in
ivp.py
(see issue [Tests] Fix Pandas related FutureWarnings #505).RuntimeError in
lq_control.py
(see issue [Tests] Fix RuntimeWarning in lqcontrol.py #506)Pandas related future warnings in
test_quad.py
(see issue [Tests] Fix FutureWarning in ivp.py #507).This is my first QuantEcon PR so let me know if I need to fix anything.