Skip to content
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

Add backward induction and beta =1 #172 #228

Merged
merged 15 commits into from
Jan 11, 2019
Merged

Conversation

tokuma09
Copy link
Contributor

@tokuma09 tokuma09 commented Dec 9, 2018

This pull request is related with #172.

I try the followings.

  • add backward_induction method and explanation.
  • allows beta = 1 for DiscreteDP`
  • write test codes for them.

close #172.

@codecov-io
Copy link

codecov-io commented Dec 9, 2018

Codecov Report

Merging #228 into master will decrease coverage by 39.02%.
The diff coverage is 70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #228       +/-   ##
===========================================
- Coverage   95.85%   56.83%   -39.03%     
===========================================
  Files          25       25               
  Lines        1061     1786      +725     
===========================================
- Hits         1017     1015        -2     
- Misses         44      771      +727
Impacted Files Coverage Δ
src/markov/ddp.jl 66.66% <70%> (-31.98%) ⬇️
src/quad.jl 8.98% <0%> (-84.96%) ⬇️
src/optimization.jl 53.06% <0%> (-46.94%) ⬇️
src/util.jl 58.88% <0%> (-39.26%) ⬇️
src/zeros.jl 42.37% <0%> (-37%) ⬇️
src/filter.jl 63.63% <0%> (-36.37%) ⬇️
src/matrix_eqn.jl 58.2% <0%> (-34.65%) ⬇️
src/markov/markov_approx.jl 64.51% <0%> (-33.86%) ⬇️
src/sampler.jl 62.96% <0%> (-31.49%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8937f8...9fb4e9c. Read the comment docs.

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
@oyamad
Copy link
Member

oyamad commented Dec 10, 2018

@sglyon What is the status of issue #136 (documentation style)? Isn't it more efficient to start following the official guidance in the new code?

@oyamad
Copy link
Member

oyamad commented Dec 12, 2018

Will you also add a test with Rational inputs?

(Define the R and Q arrays with Rational first, then run the test with it; in the second test, convert them into Float64.)

@sglyon
Copy link
Member

sglyon commented Dec 13, 2018

@oyamad I'd be happy to follow something more official.

I believe @jlperla and his team were working on a style guide. Perhaps they included a discussion of docstrings conventions to follow?

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

src/markov/ddp.jl Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
@tokuma09
Copy link
Contributor Author

Thanks. Fix them.

src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
src/markov/ddp.jl Outdated Show resolved Hide resolved
@tokuma09
Copy link
Contributor Author

Fix them.

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this is ready to merge.

@oyamad
Copy link
Member

oyamad commented Jan 11, 2019

@sglyon Can I merge this PR?

@sglyon
Copy link
Member

sglyon commented Jan 11, 2019

Looks great to me, thank you for following up.

Sorry I forgot to merge this after it was approved

@sglyon sglyon merged commit ae0046f into QuantEcon:master Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiscreteDP: Add backward_induction
4 participants