Skip to content

FIX: two minor modifications in lqcontrol #498

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

Merged
merged 2 commits into from
Aug 7, 2019

Conversation

shizejin
Copy link
Member

I made two fixes for LQ and LQMarkov classes.

  1. There was a typo in calculating the values of the last period states for LQMarkov models. I forgot to left multiply the C matrix to the vector of shocks. I have fixed this and modified tests correspondingly.

  2. The code was computing stationary values each time we call compute_series, which is unnecessary since the optimal policy F are stored as class attribute and do not change. This increases running time especially for lectures that simulate multiple paths for the same model, e.g. "How to Pay for a War: Part 1". I modified it to only compute stationary values once when compute_series method is called for the first time. By doing so, the running time of "How to Pay for a War: Part 1" decreases from 42s to 12s.

The first point is closely related to the work about the series of Markov Jump LQ DP lectures. I feel sorry that I fail to spot this typo. It would be great if we can make another release before we publish those lectures. However, since this bug only matters for values in the last period and almost has no influence on the patterns of the graphs that we draw, I guess it will also be fine that we publish lectures first and include these fixes in a release with other commits later.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 94.097% when pulling cfd14dc on shizejin:fix_lqcontrol into 15b3b93 on QuantEcon:master.

@jstac
Copy link
Contributor

jstac commented Jul 19, 2019

Thanks @shizejin, I appreciate the fix.

@oyamad @mmcky This looks good to me.

@mmcky mmcky merged commit 7ea7b18 into QuantEcon:master Aug 7, 2019
@mmcky mmcky mentioned this pull request Dec 9, 2019
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.

4 participants