FIX: ValueError LQMarkov convergence failed, Closes #508

[bktaha]

Hi!

For #508 I noticed the solver method matrix_eqn.solve_discrete_riccati_system() allowed for setting an upper bound on the number of iterations, but the stationary_values() method of LQMarkov didn't make use of this arg so it threw an error if it failed to converge within the preset number of iterations, 1000.

I've added an optional arg to the LQMarkov init method to increase the upper bound on the number of iterations, and it was able to converge for the betas mentioned in #508 ( converged for 0.973 in 1001 iterations, for 0.974 in 1050 and for 0.99 in 2700 ).

I didn't add a test for this since this was a small enough change, but I can add it if needed.

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-10 00:26:18 UTC

[coveralls]

Coverage remained the same at 93.919% when pulling 719ffbc on bktaha:fix-lqmarkov-maxiter into 9d0fb5b on QuantEcon:master.

[mmcky]

thanks @bktaha.

@oyamad are you happy with adding max_iter=1000 as a default value. This is outside of my domain knowledge so wanted to check with you on this.

[mmcky]

@bktaha can you update the docstring with this optional argument

QuantEcon.py/quantecon/lqcontrol.py

Line 66 in 9d0fb5b

Parameters

[oyamad]

I would like to delegate it to the author @shizejin.

@shizejin Will you review this PR?

[shizejin]

Sure @oyamad.

Thanks @bktaha, this is going to be a very meaningful improvement of the code. One minor point is that max_iter is purely a computational parameter, which means it should be a keyword argument of the method stationary_values instead of the LQMarkov class. It would be even better if you would be willing to take care of the indentations in line432 and line521.

I agree with you that a test for this is not needed.

[bktaha]

Thanks @shizejin for the kind comments, and for reviewing this PR.

As you suggested, I've moved the max_iter parameter to the stationary_values method and fixed the docstring to reflect that change. The line indent issue is taken care of as well.

[shizejin]

Thanks @bktaha for your quick response. The PR looks great to me except that I think the following way of breaking a line might be favorable:

Ps = solve_discrete_riccati_system(Π, As, Bs, Cs, Qs, Rs, Ns, beta,
                                   max_iter=max_iter)

The difference between this and the previous version of your code is that one space before max_iter is deleted so that the starting of the second line is aligned with (. But this is no more than my personal opinion and is really a minor point which does not affect the good quality of your commit. It would be great to have opinions from @oyamad and @mmcky, and I recommend merge for this PR after this is set.

[mmcky]

thanks @bktaha, @shizejin and @oyamad.

I agree with @shizejin style comment on line breaks. It is in line with common python practice.
@bktaha thanks so much for submitting and being willing to iterate on this PR. It is greatly appreciated. Once that minor update has been done I will merge.

[bktaha]

@mmcky I've made the suggested style fix.

[mmcky]

thanks @bktaha for the PR -- really appreciate the contribution!

Thanks @shizejin and @oyamad for your input and comments.