Add quadrature method #198
Conversation
Codecov Report
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
+ Coverage 90.72% 91.02% +0.29%
==========================================
Files 24 24
Lines 1682 1693 +11
==========================================
+ Hits 1526 1541 +15
+ Misses 156 152 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks great!
I have a few minor changes that I requested for you to make.
Also, I was wondering if we really need to introduce the FastGaussQuadrature dependency? Do the quadrature routines inside QuantEcon not satisfy the requirements? If they do work, did you opt for FastGaussQuadrature for performance reasons?
src/markov/markov_approx.jl
Outdated
| @@ -13,6 +13,7 @@ https://lectures.quantecon.org/jl/finite_markov.html | |||
|
|
|||
| import Optim | |||
| import NLopt | |||
| import FastGaussQuadrature: gausshermite | |||
There was a problem hiding this comment.
We should change this to using FastGaussQuadrature: gausshermite. Import has two uses:
- Bring a module name into scope (as we do with Optim and NLopt above)
- Bring a specific function from a different module into scope so we can extend the function with our own methods. We don't add methods to
gausshermite, so we should just callusinginstead ofimport.
| @@ -371,17 +375,17 @@ function discrete_var(b::Union{Real, AbstractVector}, | |||
| # Maximum entropy optimization | |||
| if n_moments == 1 # match only 1 moment | |||
| temp[:, jj], _, _ = discrete_approximation(y1D[jj, :], | |||
| X -> (reshape(X, 1, Nm)-cond_mean[jj, ii])/scaling_factor[jj], | |||
| X -> (X'-cond_mean[jj, ii])/scaling_factor[jj], | |||
There was a problem hiding this comment.
Do ay of these operations need . in front of them to make them broadcasting?
There was a problem hiding this comment.
hmmm..., I don't think so. X is always vector, cond_mean[jj, ii] and scaling_factor[jj] are always scalar.
Does it answer your question?
src/markov/markov_approx.jl
Outdated
| return nothing | ||
| end | ||
| warn_persistency(B::Real, method::Quadrature) = warn_persistency(fill(B, 1, 1), method) | ||
| warn_persistency(::Union{Real, AbstractMatrix}, ::Union{Even, Quantile}) = nothing |
There was a problem hiding this comment.
Instead of ::Union{Even, Quantile}, let's have the second argument be ::VAREstimationMethod so that is the default for any additional VAREstimationMethod subtypes we might create in the future.
|
@sglyon I removed |
|
Sounds great, thanks for the update! |
NOT READY FOR REVIEW The code seems to be working well basically, but let me finalize docs and potentially brush up the codeEDIT: Ready for review
This PR adds quadrature grid method for
discrete_var, which is the last one of the three methods