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

Smith Wilson yield #37

Merged
merged 10 commits into from
Oct 23, 2021

Conversation

kasperrisager
Copy link
Contributor

Example of the Smith-Wilson yield curve included into Yields.jl. This is a draft for discussing ideas, so it's a little light on testing, doc and performance optimizations.

It thrown together from code I had lying around, and I didn't make a big effort to make it look Yields.jl'ish. The CalibrationInstruments struct makes a lot of sense for the SW curve, since the calibration neatly reduces to a linear algebra problem, but for e.g. bootstrapping, the structure would have to be different (as a start, you would want instruments ordered by expiry).

Added the Smith-Wilson curve with a simple test.
Adds CalibrationInstruments that can be used for calibrating a SmithWilsonYield
Add quote structs for different instrument types with conversion to CalibrationInstruments
@alecloudenback
Copy link
Member

alecloudenback commented Oct 4, 2021

This looks like a great start, and I apprciate already addings tests.

I think to be most effective in reviewing and figuring out how this fits in with the rest of the package, I need to first follow what SW is doing and found the following resources which could be sources of additional information and test cases:

I translated the linked example above into Julia to help me follow along. I still need to digest what each step was doing, as thus far it was more of a translation exercise.

@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #37 (5d3646b) into master (623aee4) will increase coverage by 1.03%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   95.26%   96.29%   +1.03%     
==========================================
  Files           1        1              
  Lines         169      243      +74     
==========================================
+ Hits          161      234      +73     
- Misses          8        9       +1     
Flag Coverage Δ
unittests 96.29% <97.33%> (+1.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Yields.jl 96.29% <97.33%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 623aee4...5d3646b. Read the comment docs.

src/smith_wilson.jl Outdated Show resolved Hide resolved
Comment on lines 54 to 58
struct CalibrationInstruments
t # Column vector of maturities
cf # Matrix of cash flow for each [maturity, instrument]
p # Row vector of instrument prices
end
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be used aside from constructing a SW curve, nor is it used for dispatch. Would it be simpler or easier to remember to pass a tuple or named arguments instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original intent was that there would be an AbstractCalibrationInstruments where you could also do things like iterate over the cash flows and put that into a bootstrapping algorithm. But I agree that until that exists, it's going to look over-engineered to have a struct for just the sw curve.

Copy link
Member

Choose a reason for hiding this comment

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

I think it brings up the question about pulling basic asset instruments into its own set of types and/or package? There's a lot more that could be explored there, like basic asset modeling. I don't think we have to hold up SW yields for that, but it could be a near term feature to build out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably not a bad idea, it'd be quite useful to have that (and fun to make, hopefully 😃 ). But I'll focus on wrapping this up as a minimal stand-alone pr and address your feedback. Thanks for comments and thoughts so far!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a quick status: I reviewable PR is not far away now, expected to be 300-400 lines of code + a bit of README. I suppose it's best to put it back into Yields.jl and runtests.jl. I think I can better find my way around those files now 😃.

@alecloudenback
Copy link
Member

but for e.g. bootstrapping, the structure would have to be different (as a start, you would want instruments ordered by expiry).

My understanding is that one of the key advantages of SW is that it's analytically solved with no boostrapping. When would this be combined with bootstrapping?

Some things to do before merging:

  • Add some more test cases (the links I posted above have some numeric test cases)
  • Add accumulation function (can just be inverse of discount)
  • Add note on the ReadMe about this functionality

export InstrumentQuotes, ZeroCouponQuotes, SwapQuotes, BulletBondQuotes

"""
SmithWilsonYield(ufr,alpha,u,qb)
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to just call this SmithWilson. I don't think there's an ambiguity issue, and reads better when imported elsewhere, e.g.:

import Yields

Yields.SmithWilson(...)

Copy link
Member

Choose a reason for hiding this comment

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

Also, the literature seems to be so consistent in the use of α and Julia make it so easy to enter unicode, I think we should just use α instead of alpha

@alecloudenback
Copy link
Member

Another area I'd like to consider: should the rest of the package just reference the instruments defined here?

E.g. instead of:

curve = Yields.Zero(rates,maturities)

it would be the following for bootstrapping:

curve = boostrap(zcp::ZeroCouponPrices) #change the current ZeroCouponQuotes to  Prices
curve = bootstrap(zcp::ZeroCouponRates) # using a set of market rates instead of prices

or SW

curve = SmithWilson(ufr,alpha,zcp)

@alecloudenback
Copy link
Member

This is sparking a lot of really great ideas, thanks for the PR again!

Brings the addition of the Smith-Wilson yield curve up to a reviewable state.
@kasperrisager kasperrisager marked this pull request as ready for review October 21, 2021 08:39
@kasperrisager
Copy link
Contributor Author

I've tried to keep it as unsurprising as possible based on above discussions. Main changes

  • More type annotation to prevent unintended use.
  • Remove the CalibrationInstruments struct but kept the instrument quote structs.
  • Enforce vector lengths, etc, with inner constructors. It gets a bit clunky, but more intricate data structures would require a design discussion about how to represent cashflows, and so on.

I get lots of code reviews in my day job, so please go ahead and review in whatever style you like 😄.

src/Yields.jl Outdated
Comment on lines 513 to 517
"""
SmithWilson(ufr, α, u, qb)

Create a yield curve object that implements the Smith-Wilson interpolation/extrapolation scheme.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind document what the meaning of each of the arguments is?

@alecloudenback
Copy link
Member

alecloudenback commented Oct 22, 2021

A few tweaks:

  • prefer full name frequency to freq for consistency with rest of package
  • use dispatch to make calls to cashflows less verbose by dispatching on the quoted instruments
  • split up bullet_cashflows (which reutrns a tuple of (times,cashflows) into two functions cashflows and timepoints. This is to be consistent with the API in LifeContingencies.jl, which uses cashflows and timepoints to effectively do the same thing on some liability structs.

One solid request and then a question before merging:

  1. Request: If you could just add some more detail to the docstring for SmithWilsonCurve that would help orient users as to how to construct with the pieces.
  2. Question: ufr and α aren't used for dispatch, and I found hard to remember the order of arguments. This makes both of those arguments good candidates for being keyword arguments. So the API when using the quoted instruments would be: SmithWilson(swq, α=0.1,ufr=0.05) instead of SmithWilson(0.1,0.05,swq). What do you think about making that change?

@kasperrisager
Copy link
Contributor Author

Thanks for tweaking, much appreciated ❤️.

  1. Will do
  2. Makes a lot of sense to do it that way - there's a big risk in inadvertently switching those two arguments. I was thinking, down the line, that ufr should actually be ::Rate, but as long as it's an Any I agree it's safer to have it as a keyword argument (together with α). In normal use, quotes would vary more often than α which would vary more often than UFR, so I'd put them in that order in the signature.

Commit on it's way. I can also see I missed a little test coverage, so I'll add that too.

kasperrisager and others added 4 commits October 22, 2021 15:19
Made alpha and ufr keyword arguments in convenience constructors for Yields.SmithWilson
@alecloudenback
Copy link
Member

I made a few final tweaks to docstrings and made the u/qb method use keyword args as well for API consistency. Locally I couldn't get the generated docs to show the new methods. I'm hoping that the Github actions will catch them and build it into the Dev Docs.

@alecloudenback
Copy link
Member

alecloudenback commented Oct 23, 2021

Now Registered as available package update! Also now mentioned on the JuliaActuary.org site: https://juliaactuary.org/#yieldsjl

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.

2 participants