search

tnagler / VineCopula

Statistical inference of vine copulas
88 stars 32 forks source link

BiCopEst for boundary case #63

Closed notEvil closed 4 years ago

notEvil commented 4 years ago

Hi,

https://github.com/tnagler/VineCopula/blob/20aea996717ebb797860e9c8ee57c1ab355e69d0/R/BiCopEst.R#L176 fails explicitly if empirical tau is outside of valid range (e.g. tau = -epsilon for Clayton). At least for MLE, parameters should be moved to closest boundary (e.g. 1e-4 for tau = -epsilon for Clayton) in those cases imo.

KR, Andreas

ulf85 commented 4 years ago

Seems to be a numerical issue. Such boundary cases are tricky. Any recommendations?

notEvil commented 4 years ago

I would add an argument to BiCopTau2Par which allows Kendall's tau to be moved to closest valid value, for instance mode=c('exact', 'approximate'), and let BiCopEst always use 'approxmiate'. Even if Kendall's tau is completely off (say -0.7 for Clayton), the estimate is still the best among all valid estimates.

tnagler commented 4 years ago

I don't really like to add an option for this (there are zero users for 'exact'). I'd make the function behave like your 'approximate'.

It will take me a while until I get to all the issues here, I am happy to accept pull requests though ;)

notEvil commented 4 years ago

I'll give it a try :)

notEvil commented 4 years ago

Check out https://github.com/notEvil/VineCopula/commit/89b614cb6673d5f2f58bb68b3f8448ce7f616430 Its not ready for pull yet because there are a couple of open points:

(While I checked the code I saw that BiCopEst and BiCopEst.intern are almost identical. Probably worth refactoring.)

tnagler commented 4 years ago

I don't think check.taus is obsolete. Kendall's tau should only be adjusted within BiCopEst(.intern). If someone calls BiCopTau2Par(4, -0.5) or BiCop(4, tau = -0.5) there should still be an error.

ulf85 commented 4 years ago

Yes, checking is still necessary. Further, I the changes have to be backward compatible. I would change as little as possible. Just enough that it does not break functions in the boundary cases. Thus an approximation at the boundary cases seems to be a possible solution. @notEvil thanks for the suggestions

notEvil commented 4 years ago

Sry I misunderstood. I've reverted the changes, added adjustTaus to file BiCopCheck.R where adjustPars already resides and added calls to adjustTaus in BiCopEst and BiCopEst.intern; see https://github.com/notEvil/VineCopula/commit/94008969911e3ce9eda6590047eecc2a96ce7266 (check diff against origin/master) Hope thats more in line with what you were thinking about.

tnagler commented 4 years ago

That looks good, many thanks for your effort! You can open a PR and I'll merge as soon as the checks pass.