Add filer and smoothing for regime switching model by Shunsuke-Hori · Pull Request #250 · QuantEcon/QuantEcon.jl

Shunsuke-Hori · 2019-10-08T22:10:54Z

Filtering and smoothing features for regime switching model are added. Some tests for them are added too.

cc7768

This looks cool! Thanks for adding this @Shunsuke-Hori

I haven't walked through and double checked the math, but have added some code comments.

Are the tests based on other code that make you relatively confident they're correct?

cc7768 · 2019-10-11T20:10:53Z

 end
+
+"""
+- `g::Function`: Parameterized `Function` that recieves three arguments 


We should break these lines a little more frequently than they're currently broken -- I don't remember what the maximum number of characters per line we decided to enforce for Jula though.

@sglyon should remember.

@cc7768 It totally slipped my mind. I think it is 80.

cc7768 · 2019-10-15T14:30:46Z

+mutable struct RegimeSwitchingModel{TD <: AbstractArray, TP <: AbstractMatrix, TPS <: AbstractMatrix}
+    g::Function
+    y::TD
+    parameter


Is there a reason that we don't want this typed? By having a mutable structure and not having this argument typed, we can change it to anything we want -- For example, I can set rsm.parameter = 1 and it won't complain. Is there a case where this changes types?

Is there a reason that we don't want this typed?

Kind of, I thought some may want to use user-defined struct like this rather than NamedTuple. But I also think it should be typed. Maybe I can force it to be NamedTuple.

Perfect -- This is a use case Julia should handle very cleanly! You can add a type parameter to RegimeSwitchingModel and then just refer to that type parameter when typing the argument. I'd make suggestion, but I can only figure out how to suggest one line change, but here's what it would look like:

mutable struct RegimeSwitchingModel{TD <: AbstractArray, TP <: AbstractMatrix, TPS <: AbstractMatrix, TPRM} g::Function y::TD parameter::TPRM ...

This will allow the functions to specialize to the user's type (there will be a new type of RegimeSwitchingModel for every type that gets put in as a parameter) and still prevent people from doing "weird" changes to the parameter data.

Oh I see, that makes sense and is way better. Thank you!!

cc7768 · 2019-10-15T14:32:25Z

+                                   period 0.
+
+##### Returns
+- `p`: likelihood at each period


It wouldn't hurt to emphasize that the returns are each of size T x M where T is the number of observations and M is the number of regimes.

That's fair. I'll do so.

cc7768 · 2019-10-15T14:35:25Z

+    T = size(y, 1)
+    p = Vector{Float64}(undef, T)
+    logL = 0
+    for t in 1:T


I like how the code is structured that allows you to write such a simple (and beautiful!) for-loop to do the core of the computation.

cc7768 · 2019-10-15T14:38:21Z

+            rsm.prob_smoothed[t, s] = sum(p_s_joint_smoothed[t, s, :])
+        end
+    end
+    verbose ? (return p_s_joint_smoothed) : (return nothing) 


Does this if statement in return cause confusion to the compiler? This could prevent it from inferring the type of the output of this function. Is there a reason to not just return p_s_joint_smoothed?

I don't think it does create a problem -- It identifies it as a union of nothing and the array type:

julia> @code_warntype test(rsm, true) Variables #self#::Core.Compiler.Const(test, false) rsm::Main.QuantEcon.RegimeSwitchingModel{Array{Float64,2},Array{Float64,2},Array{Float64,2}} verbose::Bool Body::Union{Nothing, Array{Float64,1}} 1 ─ %1 = Main.randn(10)::Array{Float64,1} │ %2 = (:verbose,)::Core.Compiler.Const((:verbose,), false) │ %3 = Core.apply_type(Core.NamedTuple, %2)::Core.Compiler.Const(NamedTuple{(:verbose,),T} where T<:Tuple, false) │ %4 = Core.tuple(verbose)::Tuple{Bool} │ %5 = (%3)(%4)::NamedTuple{(:verbose,),Tuple{Bool}} │ %6 = Main.QuantEcon.smooth!::Core.Compiler.Const(Main.QuantEcon.smooth!, false) │ %7 = Core.kwfunc(%6)::Core.Compiler.Const(getfield(Main.QuantEcon, Symbol("#kw##smooth!"))(), false) │ %8 = Main.QuantEcon.smooth!::Core.Compiler.Const(Main.QuantEcon.smooth!, false) │ %9 = (%7)(%5, %8, rsm, %1)::Union{Nothing, Array{Float64,1}} └── return %9 julia> @code_warntype test(rsm, false) Variables #self#::Core.Compiler.Const(test, false) rsm::Main.QuantEcon.RegimeSwitchingModel{Array{Float64,2},Array{Float64,2},Array{Float64,2}} verbose::Bool Body::Union{Nothing, Array{Float64,1}} 1 ─ %1 = Main.randn(10)::Array{Float64,1} │ %2 = (:verbose,)::Core.Compiler.Const((:verbose,), false) │ %3 = Core.apply_type(Core.NamedTuple, %2)::Core.Compiler.Const(NamedTuple{(:verbose,),T} where T<:Tuple, false) │ %4 = Core.tuple(verbose)::Tuple{Bool} │ %5 = (%3)(%4)::NamedTuple{(:verbose,),Tuple{Bool}} │ %6 = Main.QuantEcon.smooth!::Core.Compiler.Const(Main.QuantEcon.smooth!, false) │ %7 = Core.kwfunc(%6)::Core.Compiler.Const(getfield(Main.QuantEcon, Symbol("#kw##smooth!"))(), false) │ %8 = Main.QuantEcon.smooth!::Core.Compiler.Const(Main.QuantEcon.smooth!, false) │ %9 = (%7)(%5, %8, rsm, %1)::Union{Nothing, Array{Float64,1}} └── return %9

It does add "complexity" relative to return p_s_joint_smoothed though, so is there a reason not to return it always?

is there a reason not to return it always?

No, when I realized p_s_joint_smoothed should be returned, I left return nothing case so that I didn't need to make change to existing parts, but that was a silly idea. Now I agree with you, it's just confusing. I'll fix it.

Shunsuke-Hori · 2019-10-16T02:53:11Z

@cc7768 Thank you for your review!! Your comments improve the codes a lot.

The test is based on python statsmodels, which I assumed correct. (https://www.statsmodels.org/dev/examples/notebooks/generated/markov_autoregression.html)

It seems I was more stupid than I thought. I'll update the codes soon.

cc7768 · 2019-10-17T01:47:47Z

Excellent -- That sounds like a great place to find a test for the code :)

Many thanks for putting this together and for addressing the "nitpicks" that I have.

Shunsuke-Hori · 2019-11-17T04:21:38Z

I updated the code. Could anyone do another round of review?

cc7768

I think this looks good now. Would be helpful if someone else took a review -- Maybe @sglyon can have a look and then merge when he's happy.

Shunsuke-Hori added 15 commits May 23, 2019 12:53

initial commit (need to add test and docs)

134b51b

rename and small fix

bbe317d

add test for linear regime switching filter

cf44482

minor update on the test

434c4d8

fix code for smoothing and add test for it

83f0401

add docs

42fdb18

force P to be square matrix

fe1c914

fix reference to P

1fa42bd

make default smoomthing algorithm Kim algorithm

b50f6b1

add verbose option

eefba9c

make verbose keyword and fix reference problem

9e0e884

test old code too

9be82ac

reorganize api

9639d93

make filer! accept keyword argument

666e265

small performance improvement

6fce054

Shunsuke-Hori requested review from cc7768 and sglyon October 8, 2019 23:29

cc7768 requested changes Oct 15, 2019

View reviewed changes

Shunsuke-Hori added 2 commits November 15, 2019 14:54

update docs, make changes following the comments

99d5ca5

a little more line breaks

601bdf8

cc7768 reviewed Nov 26, 2019

View reviewed changes

Shunsuke-Hori mentioned this pull request Sep 21, 2020

Feature request: standard econ filters/smoothers #131

Open

Uh oh!

Conversation

Shunsuke-Hori commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cc7768 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shunsuke-Hori commented Oct 16, 2019

Uh oh!

cc7768 commented Oct 17, 2019

Uh oh!

Shunsuke-Hori commented Nov 17, 2019

Uh oh!

cc7768 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Shunsuke-Hori commented Oct 8, 2019 •

edited

Loading