Safe InterestRate Arithmetic

[FlyingTwigster]

As discussed in issue #1414, I've written special overloaded operators for InterestRate objects and changed many parts of the code base where this operator could be used. All of these changes were in Stochastic process classes, but I'll happily change more if requested.

[pcaspers]

How does that impact performance though?

[coveralls]

Coverage decreased (-0.003%) to 71.169% when pulling 920f3ff28ad79676d370c2d968c1407362a263a5 on FlyingTwigster:saferate into c6d30f25bc188646e3036fa4f1cc1b9d4af3cd61 on lballabio:master.

[sweemer]

From a performance perspective, it would be best if DayCounter, Compounding, and Frequency could all be template parameters of the InterestRate class - then adding and subtracting different types would be a compile-time error and no runtime checks would be required. But obviously this cannot be changed in a backwards-compatible way.

Perhaps as a compromise between safety and performance we can enable these checks only when QL_EXTRA_SAFETY_CHECKS is set to true?

[auto-differentiation-dev]

From a performance perspective, it would be best if DayCounter, Compounding, and Frequency could all be template parameters of the InterestRate class - then adding and subtracting different types would be a compile-time error and no runtime checks would be required. But obviously this cannot be changed in a backwards-compatible way.

Perhaps as a compromise between safety and performance we can enable these checks only when QL_EXTRA_SAFETY_CHECKS is set to true?

The performance cost of comparing Compounding and Frequency would be relatively small if the implementation of the operators were in the header (so they can be inlined). The InterestRate class is not polymorphic and calls to retrieve these values are all inline. It would just boil down to 2 integer comparisons.

DayCounter is different though, as comparing it would involve shared pointer indirection, a polymorphic call (name method) and a string comparison. So only enabling the checks with QL_EXTRA_SAFETY_CHECKS makes sense...

[pcaspers]

Yes. It's a lot of baggage to wrap a rate into an InterestRate object in the first place. I'd almost say one could consider to go back to double and have some pure functions to do the conversions and the compounding and whatever.

It's similar to "Money". A bit too much "OO" for practical intends and purposes.

[github-actions[bot]]

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

[lballabio]

Hmm, I'm not sure on this one. The check should only be enabled by QL_EXTRA_SAFETY_CHECKS, that's for sure. But maybe the whole operator might be only defined in that case.

Or maybe we might just forget about it. Seeing where this operator is used in the PR, I'd say that instead of turning

return riskFreeRate_->forwardRate(t,t1,Continuous,NoFrequency,true).rate()
             -  dividendYield_->forwardRate(t,t1,Continuous,NoFrequency,true).rate() - 0.5 * sigma * sigma;

into

return (riskFreeRate_->forwardRate(t,t1,Continuous,NoFrequency,true)
             -  dividendYield_->forwardRate(t,t1,Continuous,NoFrequency,true) ).rate() - 0.5 * sigma * sigma;

I would rather define one or two local inline functions and write

return forward(riskFreeRate_,t,t1) - forward(dividendYield_,t,t1) - 0.5 * sigma * sigma;

or even

return r(t,t1) - q(t,t1) - 0.5 * sigma * sigma;

and sidestep the problem entirely.

In general I tend to agree with Peter — InterestRate is a bit too much. Unfortunately, going back to double would probably be too much disruption...

[github-actions[bot]]

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

[github-actions[bot]]

This PR was automatically closed because it has been stalled for two weeks with no further activity.