Add `LQMarkov`. #489

shizejin · 2019-07-02T15:08:49Z

Integrating the code for solving Markov jump LQ dynamic programming problems from the notebook in the sandpit repository.

The modifications I have made comparing to the original code:

modify the notations to be consistent with the existing LQ class,
discard the usage of dict and adopt ndarray to store transition, payoff matrices, policies, and value function arrays,
arrange the part of matrix calculation to make it easier to read and more efficient.
add tests.

Here is a demonstration for this qe.LQ_Markov class.

Two points that I would like to have your opinions before I move on:

the original code only applies to the case with infinite horizon. I am wondering if I should try to add code for finite horizon case as well?
parallelization over Markov states during iterations of the stacked system of discrete riccati equations could be very beneficial for the cases with large number of Markov states. Shall we try to explore whether we want to implement this or not?

I will add more tests in the next few days.

pep8speaks · 2019-07-02T15:08:53Z

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

In the file quantecon/matrix_eqn.py:

Line 303:29: E127 continuation line over-indented for visual indent
Line 304:29: E127 continuation line over-indented for visual indent

Comment last updated at 2019-07-05 03:58:57 UTC

coveralls · 2019-07-02T15:21:47Z

Coverage increased (+0.2%) to 94.111% when pulling 5fea2de on shizejin:add_Markov_LQ into a787600 on QuantEcon:master.

jstac · 2019-07-02T17:40:41Z

quantecon/lqcontrol.py

+    deterministic (and :math:`C` is set to a zero matrix of appropriate
+    dimension).
+
+    The optiaml value function :math:`V(x_t, s_t)` takes the form


jstac · 2019-07-02T17:42:10Z

quantecon/lqcontrol.py

+
+    def __init__(self, Π, Qs, Rs, As, Bs, Cs=None, Ns=None, beta=1):
+
+        # == Make sure all matrices can be treated as 3D arrays == #


Here I was trying to see that all the arrays (Qs, Rs, etc.) consist of 2d arrays, but I guess it is confusing. Would "Make sure all matrices for each state are 2D arrays" be a little better?

No, I just mean this: In your comment on line 423, you say 3D but do you actually intend to say 2D?

I actually intended to say 3D since I was referring to the shape of Qs array (and Rs etc.) and Qs has shape m (number of Markov states) x n (number of state variables) x n, but after you pointed this out, I realized Qs is not a matrix and saying "matrices are 3D arrays" does not make any sense.. so I think maybe the comment I proposed above would be a little more clear

jstac · 2019-07-02T17:48:10Z

Beautifully written code. Great work @shizejin

Regarding suggestions 1--2, let's wait and see if there's demand before we make those changes (but thanks for the suggestions).

Rather than those additions, please have a think about the tests and see if you can keep coverage high.

shizejin · 2019-07-03T10:53:00Z

Hi @jstac, I added more tests and the newly added lines in 4 changed files are fully covered. I think this is ready to be merged :)

shizejin · 2019-07-03T15:56:14Z

It seems that the coverage rates increase or stay the same for changed files, but the total coverage rate decreases by 0.9%. I am not sure what is happening here...

jstac · 2019-07-03T18:05:31Z

Thanks @shizejin .

As far as I can tell, you don't have a test for solve_discrete_riccati_system. Could that be the reason coverage goes down?

shizejin · 2019-07-04T04:41:20Z

Thanks @jstac for your suggestion! I had a look at the coverage report and the lines in solve_discrete_riccati_system are fully covered. This is because this function is called in LQ_Markov and I am testing for all the possible situations (raising erros etc.), so I think this should be fine.

I think the following is what causes our confusion:

For each PR, we run two build jobs on Travis, one for linux and one for osx. Both jobs submit coverage reports after success, and the coverage badge is updated by the one that is submitted earlier. Usually the linux build job runs faster, but yesterday the coverage report from osx build job is submitted earlier. In details, this "Add LQ_Markov" PR will update the coverages by

linux: 93.95 -> 94.11,
osx: 92.86 -> 93.05.

But since osx coverage report is submitted earlier, the coverage badge is updated by

badge: 93.95 -> 93.05.

This is why we observe a decrease in the coverage.

I am trying to fix this by #490, which modifies Travis file to make sure that we only update coverage badge by reports from the linux build job. By doing so, I think the coverage updates can be more consistent.

oyamad · 2019-07-04T04:57:46Z

Just a comment on naming: I guess in Python usually underscores are not used in a class name; if that is the case, would LQMarkov be better?

According to PEP8:

Class names should normally use the CapWords convention.

Does "CapWords convention" exclude use of an underscore?

jstac · 2019-07-04T05:18:14Z

@shizejin Good plan!

@oyamad You are right. @shizejin , could you please change the class name as @oyamad suggests?

shizejin · 2019-07-04T05:29:07Z

Thanks @oyamad for your suggestion!

@jstac sure, I will modify the PR after #490 is merged.

shizejin · 2019-07-05T04:21:10Z

Hi @jstac @oyamad, would you mind to let me know if you have any other comments or suggestions? Otherwise I think this is ready to merge :)

jstac · 2019-07-05T04:31:36Z

Very nice work. I approve this PR.

jstac · 2019-07-08T00:11:26Z

@mmcky It would be good to have this merged soon, as we'll follow it up with some new lectures. Please have a look when you can.

Thanks.

mmcky · 2019-07-08T00:49:00Z

thanks @shizejin and @jstac.

jstac reviewed Jul 2, 2019

View reviewed changes

shizejin force-pushed the add_Markov_LQ branch 3 times, most recently from 3e75ac6 to dcad72a Compare July 3, 2019 09:37

shizejin changed the title ~~[WIP] Add LQ_Markov.~~ Add LQ_Markov. Jul 3, 2019

shizejin force-pushed the add_Markov_LQ branch from 76a43a4 to dcad72a Compare July 4, 2019 04:08

shizejin mentioned this pull request Jul 4, 2019

TRAVIS: Set coverage branch as linux. #490

Merged

shizejin force-pushed the add_Markov_LQ branch 2 times, most recently from bb9b784 to 976ec7b Compare July 4, 2019 06:26

shizejin changed the title ~~Add LQ_Markov.~~ Add LQMarkov. Jul 4, 2019

shizejin force-pushed the add_Markov_LQ branch 2 times, most recently from 976ec7b to 9bf2c24 Compare July 4, 2019 07:04

shizejin mentioned this pull request Jul 4, 2019

Tests of rouwenhorst fails occasionally #491

Closed

shizejin force-pushed the add_Markov_LQ branch 2 times, most recently from c1f4922 to 6924613 Compare July 5, 2019 02:59

Add LQMarkov.

5fea2de

shizejin force-pushed the add_Markov_LQ branch from 6924613 to 5fea2de Compare July 5, 2019 03:58

mmcky merged commit 1da28de into QuantEcon:master Jul 8, 2019

oyamad mentioned this pull request Apr 12, 2020

FEAT: Add AMF_LSS_VAR #476

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `LQMarkov`. #489

Add `LQMarkov`. #489

shizejin commented Jul 2, 2019

pep8speaks commented Jul 2, 2019 •

edited

Loading

coveralls commented Jul 2, 2019 •

edited

Loading

jstac Jul 2, 2019

shizejin Jul 2, 2019

jstac Jul 2, 2019

shizejin Jul 2, 2019

jstac Jul 3, 2019

shizejin Jul 3, 2019

jstac commented Jul 2, 2019

shizejin commented Jul 3, 2019

shizejin commented Jul 3, 2019

jstac commented Jul 3, 2019

shizejin commented Jul 4, 2019

oyamad commented Jul 4, 2019

jstac commented Jul 4, 2019

shizejin commented Jul 4, 2019

shizejin commented Jul 5, 2019

jstac commented Jul 5, 2019

jstac commented Jul 8, 2019

mmcky commented Jul 8, 2019


		def __init__(self, Π, Qs, Rs, As, Bs, Cs=None, Ns=None, beta=1):

		# == Make sure all matrices can be treated as 3D arrays == #

Add LQMarkov. #489

Add LQMarkov. #489

Conversation

shizejin commented Jul 2, 2019

pep8speaks commented Jul 2, 2019 • edited Loading

Comment last updated at 2019-07-05 03:58:57 UTC

coveralls commented Jul 2, 2019 • edited Loading

jstac Jul 2, 2019

Choose a reason for hiding this comment

shizejin Jul 2, 2019

Choose a reason for hiding this comment

jstac Jul 2, 2019

Choose a reason for hiding this comment

shizejin Jul 2, 2019

Choose a reason for hiding this comment

jstac Jul 3, 2019

Choose a reason for hiding this comment

shizejin Jul 3, 2019

Choose a reason for hiding this comment

jstac commented Jul 2, 2019

shizejin commented Jul 3, 2019

shizejin commented Jul 3, 2019

jstac commented Jul 3, 2019

shizejin commented Jul 4, 2019

oyamad commented Jul 4, 2019

jstac commented Jul 4, 2019

shizejin commented Jul 4, 2019

shizejin commented Jul 5, 2019

jstac commented Jul 5, 2019

jstac commented Jul 8, 2019

mmcky commented Jul 8, 2019

Add `LQMarkov`. #489

Add `LQMarkov`. #489

pep8speaks commented Jul 2, 2019 •

edited

Loading

coveralls commented Jul 2, 2019 •

edited

Loading