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.

Sign up for GitHub

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

Jump to bottom

add docs for Methods and Solve methods #21

Merged
merged 38 commits into from
Nov 18, 2018
Merged

add docs for Methods and Solve methods #21

merged 38 commits into from
Nov 18, 2018

Conversation

tokuma09
Copy link
Contributor

@tokuma09 tokuma09 commented Oct 9, 2018

This pull request adds some documents for Methods and Solve methods in cdp.jl.

Later I will add docs for Types, contructors and Simulate methods.

Document style is based on QuantEcon.jl.

@oyamad
Copy link
Member

oyamad commented Oct 9, 2018

Thanks! For the doc style, please follow Games.jl rather than QuantEcon.jl (refer in particular to QuantEcon/GameTheory.jl#25).

@tokuma09
Copy link
Contributor Author

tokuma09 commented Oct 9, 2018

I got it. Later on, I'm going to change doc style based on Ganes.jl.

@tokuma09
Copy link
Contributor Author

I rewrite docs for ContinuousDPs.jl for all functions and structs. I think this style is based on Games.jl.

@oyamad
Copy link
Member

oyamad commented Nov 15, 2018

@tokupooh Thanks!

@tokuma09
Copy link
Contributor Author

@oyamad Sure. I'll fix them.

By the way, do we need same documents for different methods ?
For example, s_wise_max! allows several types of arguments, depending on the dimensions of the problem. However, an objective of all methods is same.(I know this function works in different way internally.)

In this pull request, I add explanation for each methods, but Julia does not provide it for all methods.

Should I follow their doc style?

@oyamad
Copy link
Member

oyamad commented Nov 15, 2018

Yes, please provide a docstring to each method. (We can elaborate later if necessary.)

@tokuma09
Copy link
Contributor Author

I fix my docs for ContinuousDPs.jl by following the style where each line with 80 characters or fewer and type declaration.

In addition, explanation is consitent internally.

oyamad
oyamad requested changes Nov 18, 2018
View reviewed changes
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. Just a few more comments.

src/cdp.jl Outdated
Implement the Bellman equation collocation method as described in Mirand and
Fackler (2002), Chapter 9.

Copy link
Member

Choose a reason for hiding this comment

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

Do not remove this blank line here and above.

src/cdp.jl Outdated
for k in N:-1:1]...
) * cdp.discount * w
A[i, :] -= ckron([vec(evalbase(cdp.interp.basis.params[k], s_next[k]))
for k in N:-1:1]...) * cdp.discount * w
Copy link
Member

Choose a reason for hiding this comment

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

Will you revert this change back to the original?

src/cdp.jl Outdated
- `res::CDPSolveResult`: Object that contains result of dynamic programming
- `s_init::Real`: initial value of state variable
- `ts_length::Integer`: Length of simulation
"""
Copy link
Member

Choose a reason for hiding this comment

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

Remove this docstring.

src/cdp.jl Outdated
- `res::CDPSolveResult`: Object that contains result of dynamic programming
- `s_init::Real`: initial value of state variable
- `ts_length::Integer`: Length of simulation
"""
Copy link
Member

Choose a reason for hiding this comment

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

Remove this docstring.

src/cdp.jl Outdated
simulate(res::CDPSolveResult, s_init::Vector, ts_length::Integer) =
simulate(Random.GLOBAL_RNG, res, s_init, ts_length)
simulate(Random.GLOBAL_RNG, res, s_init, ts_length)
Copy link
Member

Choose a reason for hiding this comment

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

Do not remove the indentation.

@tokuma09
Copy link
Contributor Author

Thanks for comments. I correct them.

@oyamad oyamad merged commit 6cc5a7a into QuantEcon:master Nov 18, 2018
@tokuma09 tokuma09 deleted the docs branch November 18, 2018 04:28
@oyamad oyamad mentioned this pull request Nov 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oyamad oyamad oyamad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tokuma09 @oyamad