DiscreteDP: Allow beta=1

[oyamad]

This will close #242.

TODO

• Add tests
• Add a function backward_induction for finite horizon problems

[coveralls]

Coverage decreased (-0.1%) to 85.698% when pulling 3bdc97e on ddp_beta_1 into b0cb89a on master.

[coveralls]

Coverage increased (+0.08%) to 85.898% when pulling b18ab8f on ddp_beta_1 into b0cb89a on master.

[oyamad]

This PR is ready for review.

[coveralls]

Coverage increased (+0.2%) to 85.979% when pulling a009367 on ddp_beta_1 into b0cb89a on master.

[cc7768]

@oyamad @jstac I will have a look at this in next week or so.

[cc7768]

I looked through all of the changes to the files and everything seems in order.

Give me another few days to play with it and look at it a bit more carefully, but this looks very good. Thanks for writing this code.

[oyamad]

Thanks @cc7768.

I added in PR QuantEcon/QuantEcon.notebooks#37 an example of a finite-horizon problem with no discounting from Miranda-Fackler Section 7.6.6.

[cc7768]

This looks good and seems to have good testing for backward induction.

One thought and then I will add a couple of line comments in the code.

• It looks like you have removed the if numba_installed statements in markov/ddp.py, but not in markov/core.py. Is there a reason you did some but not all? Additionally, there is some overlap with Remove optional numba installation code #246. When these two are merged, we should merge one and then will probably need to rebase the other before getting it merged.

[cc7768]

Here you index a row by vs[t]. Someone familiar with numpy will be able to follow this, but vs[t, :] is a bit more explicit. I don't mind, but I thought I would point this out. @mmcky @jstac do y'all have any opinion on this?

[sglyon]

If we're voting I'd go with vs[t, :]

[oyamad]

Yes, it's better to be explicit. I will rewrite it.

[oyamad]

@cc7768 Thanks for your comments.

• The if numba_installed statements in markov/core.py are to be deleted by 457578b for Add states/nodes to MarkovChain/DiGraph #237. Yes, we have to be careful depending on this or Remove optional numba installation code #246 to be merged.

[oyamad]

One (minor) issue is whether v_T is a good name. v_term may be better, for the following reason.

• This python version adopts the convention that the decisions are made at periods t = 0, 1, ..., T-1, so we say that "v[t, :] contains the value vector at period t" and the terminal value is v_{T} so that v_T is consistent with this.
• In a julia version, however, we would want to suppose that the decisions are made at periods t = 1, 2, ..., T to be able to say that "v[:, t] contains the value vector at period t". Under this convention, the terminal value will be v_{T+1} for which we cannot use v_T.

[mmcky]

@oyamad This will have merge conflicts with #246. However this PR is more involved than #246 so lets merge this issue when you're ready and then I will fix up #246

[cc7768]

I see. We can address the Julia issue there when we come to it.

This is ready to be merged once @oyamad is ready. Would there be any objection to squashing these commits into one using github's new squash for pull requests?

[oyamad]

Unless anybody has a strict preference for v_T, I am going to change it to v_term.

[cc7768]

That sounds good to me.

[coveralls]

Coverage increased (+0.2%) to 85.979% when pulling 6093186 on ddp_beta_1 into b0cb89a on master.

[oyamad]

Ready for merge. (I don't know how to do "github's new squash". Please do it.)

[mmcky]

@oyamad Thanks. @cc7768 Would you mind teaching me about the squash feature? Are you over this side at any point today? I could come over to Stern otherwise.

[mmcky]

The squash feature may be available only on new commits - so merging this now as is. thanks everyone for comments and updates.