-
Notifications
You must be signed in to change notification settings - Fork 2k
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 |
| Rate BlackAverageONIndexedCouponPricer::swapletRate() const { return swapletRate_; } | ||
|
|
||
| Rate BlackAverageONIndexedCouponPricer::capletRate(Rate effectiveCap) const { | ||
| return coupon_->localCapFloor() ? optionletRateLocal(Option::Call, effectiveCap) |
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.
The thing is that the CappedFlooredOvernightIndexedCoupon has the localCapFloor_ attribute which I think is more a property of the coupon than a property of the pricer.
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.
Hmm, I see. The Black Ibor pricer doesn't have this problem because it doesn't need other data from the coupon besides the effective cap which is passed through the interface. Let me think a bit about it. And of course you're welcome do to some thinking of your own.
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 was thinking that defining an overload of capletRate(Rate) (and similarly floorletRate(Rate)) with the signature capletRate(Rate effectiveCap, bool localCapFloor) might be a good approach. After all, these functions are only ever called by the coupon. The implementation provided by the FloatingRateCouponPricer interface could simply call this overload, passing false for the localCapFloor parameter.
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.
Agreed.
| }; | ||
|
|
||
| //! capped floored overnight indexed coupon pricer base interface | ||
| class CappedFlooredOvernightIndexedCouponPricer : virtual public OvernightIndexedCouponPricer { |
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.
This can probably be folded into OvernightIndexedCouponPricer so you can avoid the double inheritance in the Black pricers. With methods like capletPrice, floorletPrice and setCapletVolatility, OvernightIndexedCouponPricer is already supposed to be used for capped coupons too.
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.
The double inheritance is avoided because I've used virtual inheritance, but yeah at this point it makes more sense to put everything under the OvernightIndexedCouponPricer at this point since I've already pass the optionletVolatilityTermStructure in the class constructor.
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?
| highly experimental and ad-hoc. As soon as a market best practice has evolved, the pricer | ||
| should be revised. */ | ||
| class BlackOvernightIndexedCouponPricer : public CompoundingOvernightIndexedCouponPricer { | ||
| using CompoundingOvernightIndexedCouponPricer::CompoundingOvernightIndexedCouponPricer; |
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're importing the base-class constructor but you're also declaring in this class a constructor with the same signature. (Maybe it's the cause of the error in the Appveyor build?) You only need one.
| highly experimental and ad-hoc. As soon as a market best practice has evolved, the pricer | ||
| should be revised. */ | ||
| class BlackAverageONIndexedCouponPricer : public ArithmeticAveragedOvernightIndexedCouponPricer { | ||
| using ArithmeticAveragedOvernightIndexedCouponPricer::ArithmeticAveragedOvernightIndexedCouponPricer; |
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.
Here the signature is different, but do you really want to import the base-class constructor?
ql/cashflows/couponpricer.cpp
Outdated
| const ext::shared_ptr<BlackOvernightIndexedCouponPricer> overnightCouponPricer = | ||
| ext::dynamic_pointer_cast<BlackOvernightIndexedCouponPricer>(pricer_); | ||
| QL_REQUIRE(overnightCouponPricer, | ||
| "pricer not compatible with capped-floored overnight indexed coupon"); |
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.
This way you're making it impossible to define and use another capped/floored pricer which is not Black. Same below.
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.
Should I only check whether the pricer is a child of OvernightIndexedCouponPricer then?
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 think so.
| 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?
| 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.
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)