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 Hansen's Standardized Skewed T Distribution #86

Merged
merged 9 commits into from Mar 22, 2021

Conversation

chm-von-tla
Copy link
Contributor

No description provided.

@chm-von-tla chm-von-tla changed the title standardized t dist: consistenly use greek letter ν Add Hansen's Standardized Skewed T Distribution Mar 3, 2021
@chm-von-tla
Copy link
Contributor Author

The first commit is a minor style fix
The motivation for the second commit is discussed in #87

Create a standardized skewed t distribution with `v` degrees of freedom and `λ` shape parameter. `ν,λ`` can be passed
as scalars or vectors.
"""
StdSkewT(ν,λ) = StdSkewT([ν λ])
Copy link
Owner

Choose a reason for hiding this comment

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

StdSkewT(ν,λ) = StdSkewT([ν, λ])

as scalars or vectors.
"""
StdSkewT(ν,λ) = StdSkewT([ν λ])
StdSkewT(ν::Integer,λ::Integer) = StdSkewT(float(ν),float(λ))
Copy link
Owner

Choose a reason for hiding this comment

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

This can be removed. That conversion happens automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it doesn't

"""
StdSkewT(ν,λ) = StdSkewT([ν λ])
StdSkewT(ν::Integer,λ::Integer) = StdSkewT(float(ν),float(λ))
StdSkewT(v::Vector{T},λ::Vector{T}) where {T} = StdSkewT{T}(v,λ)
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't work because there is no such inner constructor. Also, it probably shouldn't work, so just remove it.

StdSkewT(v::Vector{T},λ::Vector{T}) where {T} = StdSkewT{T}(v,λ)
StdSkewT(coefs::Vector{T}) where {T} = StdSkewT{T}(coefs)

(rand(d::StdSkewT{T})::T) where {T} = (ν=d.coefs[1];λ=d.coefs[2]; quantile(d,rand(1)))
Copy link
Owner

Choose a reason for hiding this comment

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

rand(d::StdSkewT{T}) where {T} = (ν=d.coefs[1];λ=d.coefs[2]; quantile(d, rand()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this correct? I see ν and λ being assigned and then not used

function quantile(d::StdSkewT{T}, q::T) where T
ν = d.coefs[1]
λ = d.coefs[2]
xa = a(d,d.coefs)
Copy link
Owner

Choose a reason for hiding this comment

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

xa = a(typeof(d), d.coefs)

ν = d.coefs[1]
λ = d.coefs[2]
xa = a(d,d.coefs)
xb = b(d,d.coefs)
Copy link
Owner

Choose a reason for hiding this comment

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

xb = b(typeof(d), d.coefs)

hand optimized the logkernel function for StdSkewT instead of using the support
{a,b,c} functions which were wasteful since calls to them were repeated. A
speed improvement of 4x has been attained on `fit(GARCH{1,1},BG96)`
@chm-von-tla
Copy link
Contributor Author

I incorporated the changes you requested, added tests (by basically imitating the tests for the other distributions), I added a mention of the distribution in the docs (I'm not familiar with the process so I may have made mistakes) and optimized the performance (see my last post on #87, and/or commit message for 49e7858)

@s-broda
Copy link
Owner

s-broda commented Mar 22, 2021

This is starting to look good. There's just a doctest that's still failing: https://travis-ci.org/github/s-broda/ARCHModels.jl/jobs/762500760

Do you know how to address this? You basically need to update the output in src/usage.md, lines 331-362. If not, I can merge it as is and fix it myself.

Cheers
Simon

@chm-von-tla
Copy link
Contributor Author

No, I am not familiar with what's going on in the lines you mentioned. However I also spotted a different thing on those logs: after the error on line 990 it is complaining that the evaluated output differs from what is expected (there is a space missing in the function call)

@chm-von-tla
Copy link
Contributor Author

It would be better if you fixed it, I haven't used julia's documentation system before

@s-broda
Copy link
Owner

s-broda commented Mar 22, 2021

OK, I'll merge and fix it myself.

Thanks for your contribution!

@s-broda s-broda merged commit 203c56b into s-broda:master Mar 22, 2021
@chm-von-tla
Copy link
Contributor Author

Thank you for the advice along the way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants