-
Notifications
You must be signed in to change notification settings - Fork 2.1k
QL-ORE ON coupons alignment #2297
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
base: master
Are you sure you want to change the base?
QL-ORE ON coupons alignment #2297
Conversation
|
@lballabio I actually need help with the test |
|
Not a lot of time now unfortunately, but I would try checking that we're not accessing some vector out of its bounds and getting garbage numbers. |
|
I pushed a fix for the failing tests—a couple of bools were left uninitialized. I still have to do a proper review... |
|
Thank you @lballabio! I didn't have much time to debug the tests lately |
|
Apologies for the long delay. One thing: currently the pricers are defined this way: classDiagram
class FloatingRateCouponPricer
class CompoundingOvernightIndexedCouponPricer
class ArithmeticAveragedOvernightIndexedCouponPricer
class CappedFlooredOvernightIndexedCouponPricer
class BlackOvernightIndexedCouponPricer
class BlackAverageONIndexedCouponPricer
FloatingRateCouponPricer <|-- CompoundingOvernightIndexedCouponPricer
FloatingRateCouponPricer <|-- ArithmeticAveragedOvernightIndexedCouponPricer
FloatingRateCouponPricer <|-- CappedFlooredOvernightIndexedCouponPricer
CappedFlooredOvernightIndexedCouponPricer <|-- BlackOvernightIndexedCouponPricer
CappedFlooredOvernightIndexedCouponPricer <|-- BlackAverageONIndexedCouponPricer
Instead, I would try to turn it into classDiagram
class FloatingRateCouponPricer
class CompoundingOvernightIndexedCouponPricer
class ArithmeticAveragedOvernightIndexedCouponPricer
class BlackOvernightIndexedCouponPricer
class BlackAverageONIndexedCouponPricer
FloatingRateCouponPricer <|-- CompoundingOvernightIndexedCouponPricer
FloatingRateCouponPricer <|-- ArithmeticAveragedOvernightIndexedCouponPricer
CompoundingOvernightIndexedCouponPricer <|-- BlackOvernightIndexedCouponPricer
ArithmeticAveragedOvernightIndexedCouponPricer <|-- BlackAverageONIndexedCouponPricer
or maybe, if we need to share something between compounded and averaged, something like classDiagram
class FloatingRateCouponPricer
class OvernightIndexedCouponPricer
class CompoundingOvernightIndexedCouponPricer
class ArithmeticAveragedOvernightIndexedCouponPricer
class BlackOvernightIndexedCouponPricer
class BlackAverageONIndexedCouponPricer
FloatingRateCouponPricer <|-- OvernightIndexedCouponPricer
OvernightIndexedCouponPricer <|-- CompoundingOvernightIndexedCouponPricer
OvernightIndexedCouponPricer <|-- ArithmeticAveragedOvernightIndexedCouponPricer
CompoundingOvernightIndexedCouponPricer <|-- BlackOvernightIndexedCouponPricer
ArithmeticAveragedOvernightIndexedCouponPricer <|-- BlackAverageONIndexedCouponPricer
What do you think? |
|
2nd and 3rd case make more sense to me. I would opt for the 3rd case. One question: is |
|
The interface is already declared in FloatingRateCouponPricer. The base class might perhaps store the volatility term structure, but otherwise there's not a lot in common, which is why the existing pricers don't have a common base. |
|
Got it. So the |
|
One problem that I have noticed @lballabio: If |
|
I think you can copy the approach in |
lballabio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, another round of comments — sorry for the piecewise review, but it's a massive PR...
| const Date& rateComputationStartDate = Date(), | ||
| const Date& rateComputationEndDate = Date()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably more for @pcaspers — these two don't seem to be used anywhere in the calculation. Should they be there at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lballabio Apologies for the rather late reply. They can be used to overwrite start and end date
Date valueStart = rateComputationStartDate_ == Null<Date>() ? startDate : rateComputationStartDate_;
Date valueEnd = rateComputationEndDate_ == Null<Date>() ? endDate : rateComputationEndDate_;
It's rarely used in practice, but there are exotic variants of on coupons where the compounding takes place over the previous (say) 3m period to get the rate for the current 3m period. We introduced these two fields to model cases like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can see that was the intent, but those two fields are never assigned to valueStart or used anywhere in the code, hence the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We assign them here to valueStart and valueEnd. Sorry, I think I am missing something or we are talking about different branches / files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see, that bit of code wasn't migrated in this PR, which makes those two variables unused here. @paolodelia99, may you have a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll take a look at it in the upcoming days. It must have been an oversight of mine.
| return spread(); | ||
|
|
||
| if (averagingMethod_ == RateAveraging::Simple) | ||
| QL_FAIL("Average OIS Coupon does not have an effectiveSpread"); // FIXME: better error message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For @pcaspers — does not have yet, or it can't be defined, or in the simple case is the same as the spread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I'd say for Simple the effective spread is equal to the spread, as you say. We don't expose a method therefore in our averaged coupon class, but no reason to fail here in my opinion.
| if (!coupon_->includeSpread()) { | ||
| finalRate += coupon_->spread(); | ||
| effectiveSpread = coupon_->spread(); | ||
| effectiveIndexFixing = finalRate; | ||
| } else { | ||
| effectiveSpread = finalRate - (compoundFactorWithoutSpread - 1.0) / tau; | ||
| effectiveIndexFixing = finalRate - effectiveSpread; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled here — in the if the effective index fixing equals the final rate and therefore inlcudes the spread, but in the else it seems to exclude it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should ask it to @pcaspers, it took this logic from ORE. But I guess in the first case, where the spread is not included in the compounding logic, the effectiveIndexFixing is set to finalRate because the observable/effective fixing used downstream is the final coupon rate after the additive spread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code in QuantExt, OvernightIndexedCouponPricer::compute(), the compoundFactor will include the spread component if and only if includeSpread == true. I.e. the same holds for rate (which is called finalRate here I think)
Rate rate = (compoundFactor - 1.0) / tau;
And for the includeSpread == true case we have to subtract the spread component to get the effective fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but for the includeSpread == false case you're setting the effective fixing to the final rate, which includes the spread (it's added just two lines above). Shouldn't the spread be excluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again maybe we are talking about different branches / files? We set indexFixing to rate here. The spread is added to swapletRate though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'm looking at the code in this PR which differs from the ORE code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then it's a question for @paolodelia99 I guess?
|
@pcaspers no hurry, just bumping this in case it fell to the bottom of your inbox and got lost, we had a few questions for you above. Thanks! |
|
I will take a look - I have flagged it in my mailbox :-) |
|
Thanks for doing all this. @paolodelia99 do you plan to reintegrate the updated ql coupon classes in ore, so that we can get rid of the QuantExt versions of these classes? |
|
Do mean in the ORE's QL fork? @pcaspers |
Ultimately yes. Maybe it is enough if you migrate the code to official ql such that we can merge this later into our ql fork and get rid of the QuantExt classes. |
|
@lballabio don't know is it better to set if (lookbackDays != 0) { //by default lookbackDays is 2147483647
BusinessDayConvention bdc = lookbackDays > 0 ? Preceding : Following;
valueStart = overnightIndex->fixingCalendar().advance(valueStart, -lookbackDays, Days, bdc); //error is throw in here: QuantLib::Error: Date's serial number (366) outside allowed range [367-109574]
valueEnd = overnightIndex->fixingCalendar().advance(valueEnd, -lookbackDays, Days, bdc);
} |
|
Hi Paolo, I'm not sure I understand the question. Right now you have |
|
I was just wondering whether to set |
This PR tries to be an alignment for the difference of the overnight coupons and ON coupons pricers in QuantLib and ORE.
Changes to note for ORE (@pcaspers ):
OvernightIndexedCouponPricercode is in theovernightindexedcouponpricer.hppfile not in theovernightindexedcoupon.hppfile anymore.rateCutoffin ORE islockoutDaysin QuantLibfixingDaysin ORE islookbackDaysin QuantLibAverageONIndexedCouponin QuantLib the averaging method is passed as a argument in theOvernightIndexedCouponand it is an enum type:OvernightIndexedCouponPricerspricing logic haven't been changes since I haven't noticed substantial differences (apart from the rateCutoff and fixingDays naming conventions). The only thing that I'm unsure of is the following line. I haven't seen such thing in QuantLib.Changes to note in QL (@lballabio ):
CappedFlooredOvernightIndexedCouponclass underovernigthindexcoupon.hpp(imported from ORE)BlackOvernightIndexedCouponPricerandBlackAverageONIndexedCouponPricerfor pricing capped / floored compounded ON coupons (imported from ORE)OvernightLegclass (includeSpread,withCaps,withFloors,withNakedOption, ...) imported from OREincludeSpread,rateComputationStartDate,rateComputationEndDatearguments to theOvernightIndexedCouponconstructor (missing args from ORE)