allow swaptions to take OvernightIndexedSwap #1593

thrasibule · 2023-02-08T16:36:07Z

closes #1588 I don't want to take any credit this for this code. This is cherry picked from https://github.com/OpenSourceRisk/QuantLib. @pcaspers mentionned before that he would be willing to contribute this to QuantLib, see #1379 (comment)

pcaspers · 2023-02-08T19:13:38Z

I have no objections doing this. Quite the contrary, it helps reducing the diff between our fork and the official repo!

coveralls · 2023-02-08T19:51:49Z

coverage: 72.497% (+0.07%) from 72.426%
when pulling 9b4efa3 on thrasibule:ois_swaption2
into c6492d0 on lballabio:master.

sweemer · 2023-02-08T22:23:33Z

ql/instruments/overnightindexedswap.cpp

@@ -72,6 +72,7 @@ namespace QuantLib {
    }

    void OvernightIndexedSwap::initialize(const Schedule& schedule) {
+        schedule_ = schedule;


Shouldn't this be assigned in the constructor?

Regardless of where it is assigned, it should be passed by value and moved into the member variable in order to avoid a copy.

sweemer · 2023-02-08T22:26:27Z

ql/instruments/swaption.hpp

@@ -97,10 +102,13 @@ namespace QuantLib {
        Settlement::Method settlementMethod() const {
            return settlementMethod_;
        }
-        Swap::Type type() const { return swap_->type(); }
+        Swap::Type type() const { return swap_ ? swap_->type() : swapOis_->type(); }


If swapOis_ is assumed to be non-null when swap_ is null then this should probably be asserted in the constructor.

It is implicitly enforced by the signature. One can never pass both a vanillaswap and a overnightindexedswap to the constructor.

sweemer · 2023-02-08T22:26:46Z

ql/instruments/swaption.cpp

+            VanillaSwap::arguments::validate();
+            QL_REQUIRE(swap, "vanilla swap not set");
+        }
+        if (swapOis) {


Do you mean else if?

sweemer · 2023-02-08T22:31:56Z

ql/pricingengines/swaption/blackswaptionengine.hpp

-        const Leg& fixedLeg = swap.fixedLeg();
+        auto swap = arguments_.swap;
+        auto swapOis = arguments_.swapOis;
+        QL_REQUIRE(swap || swapOis,


Do you mean QL_REQUIRE((swap || swapOis) && !(swap && swapOis)?

ralfkonrad · 2023-02-09T11:34:10Z

Hi @thrasibule,

IMHO this PR looks a little bit like this anti-pattern:

We are now having both swap_ and swapOis_ in Swaption and later on having (a lot of) switches to decide if the underlying swap is a VanillaSwap or an OvernightIndexedSwap.

E.g. what about the other pricing engines in ql/pricingengines/swaption? Do they still work as designed if you pass in an OvernightIndexedSwap?

@pcaspers: I guess in ORE you don't care about the other engines aka don't use them?

Wouldn't it be better to derive an OvernightIndexedSwaption from Swaption and do the needed adjustments aka switch- and if -statements there?

Regards Ralf

lballabio · 2023-02-09T11:36:31Z

Yes, I was thinking about reworking this somehow. I'll give it a go.

pcaspers · 2023-02-09T11:52:15Z

Yes - the goal in ORE was to get it working with as little effort as possible.

lballabio · 2023-03-10T11:08:53Z

I think I'd duplicate the class so that we have a Swaption and an OvernightIndexedSwaption. Same for the Black engine. We can find later ways to factor out common code (templates, maybe? Types are different).

Also, I don't think any of the other engines (besides the Black engine in this PR) will work with an OI swaption — they all use arguments_.swap one way or the other. I think this also prevents the swaption helper from working with OI, as it can calculate the Black price but not the model price. @pcaspers, did you modify those as well in ORE?

pcaspers · 2023-03-10T17:14:27Z

Yes we have adapted those as well

pcaspers · 2023-03-10T18:13:34Z

The danger here being that a partial migration and / or refactoring will cause conflicts in our codebase. Not sure if this is the right way forward?

lballabio · 2023-03-14T13:40:20Z

Hmm. Yes, we probably need to look at those as well. Do you have additional engines in QuantExt too?

tomwhoiscontrary · 2023-03-14T18:23:42Z

Another road here would be to revisit the idea of a base type for interest rate swaps (#662). I agree that it seems janky to have two fields for the different types of swaps as in the original implementation, but it seems just as janky to have two classes of swaption which ultimately differ only in the underlying index!

pcaspers · 2023-03-15T13:54:29Z

Hmm. Yes, we probably need to look at those as well. Do you have additional engines in QuantExt too?

yes - also updates for CMS pricers (not sure at the moment if they are affected too)

ralfkonrad · 2023-03-15T15:44:48Z

@pcaspers: I'm not sure I understood all of your answers concerning what has been adopted in ORE correctly. E.g. which of the engines in ql/pricingengines/swaption will currently work with the approach discussed here?

You have the BlackSwaptionEngine adjusted. But what about the others? I don't see adjustment for the FdHullWhiteSwaptionEngine, the TreeSwaptionEngine (incl. the DiscretizedSwaption).

I would expect changes for them as well to handle e.g. a Bermudan swaption which is between two call dates. The current running OIS coupon cannot be priced correctly with their current implementation, can they?!

Or have I overlooked something?

lballabio · 2023-03-15T16:08:55Z

Another road here would be to revisit the idea of a base type for interest rate swaps (#662). I agree that it seems janky to have two fields for the different types of swaps as in the original implementation, but it seems just as janky to have two classes of swaption which ultimately differ only in the underlying index!

Yes, I'm liking this solution better than any of the alternatives

pcaspers · 2023-03-15T16:22:01Z

@pcaspers: I'm not sure I understood all of your answers concerning what has been adopted in ORE correctly. E.g. which of the engines in ql/pricingengines/swaption will currently work with the approach discussed here?

You have the BlackSwaptionEngine adjusted. But what about the others? I don't see adjustment for the FdHullWhiteSwaptionEngine, the TreeSwaptionEngine (incl. the DiscretizedSwaption).

I would expect changes for them as well to handle e.g. a Bermudan swaption which is between two call dates. The current running OIS coupon cannot be priced correctly with their current implementation, can they?!

Or have I overlooked something?

No you haven't overlooked anything. We don't use FdHullWhiteSwaptionEngine or TreeSwaptionEngine in ORE, therefore I didn't adjust those. But we have other engines (not part of QuantLib) that handle Swaptions on OIS Swaps.

github-actions · 2023-05-25T01:54:27Z

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 · 2023-08-03T09:29:23Z

@pcaspers: I'm not sure I understood all of your answers concerning what has been adopted in ORE correctly. E.g. which of the engines in ql/pricingengines/swaption will currently work with the approach discussed here?
You have the BlackSwaptionEngine adjusted. But what about the others? I don't see adjustment for the FdHullWhiteSwaptionEngine, the TreeSwaptionEngine (incl. the DiscretizedSwaption).
I would expect changes for them as well to handle e.g. a Bermudan swaption which is between two call dates. The current running OIS coupon cannot be priced correctly with their current implementation, can they?!
Or have I overlooked something?

No you haven't overlooked anything. We don't use FdHullWhiteSwaptionEngine or TreeSwaptionEngine in ORE, therefore I didn't adjust those. But we have other engines (not part of QuantLib) that handle Swaptions on OIS Swaps.

@pcaspers: I'm having another look at this.

So if I get this correctly:
(a) the part of this PR that modifies the swaption itself and the Black engine can be used to price European swaptions given a vol surface built from quoted volatilities;
(b) but the part that modifies the swaption helpers can't be used until we have an engine that works with OIS swaptions (either the existing JamshidianSwaptionEngine, TreeSwaptionEngine etc or some new one from ORE) and thus can be used during calibration.

Is that right?

pcaspers · 2023-08-03T10:01:23Z

@lballabio yes that sounds right.

lballabio · 2023-09-19T09:41:31Z

@pcaspers this can probably be simplified using #1789 which is now merged—do you want to try it in ORE first?

pcaspers · 2023-09-19T12:53:28Z

@lballabio that would be nice. However since we have a release end of September I am going to look into this in October or possibly even later. I.e. I don't want to hold things up!

lballabio · 2023-09-19T13:28:53Z

Sure!

pcaspers · 2024-02-05T19:25:21Z

So in ORE we are moving in a slightly different direction now: In general we need to support swaptions with European, Bermudan, American exercise right on overnight, Ibor, BMA/SIFMA, Subperiod coupons. For Bermudan / American swaptions we are using the MultiLegOption instrument

https://github.com/OpenSourceRisk/Engine/blob/master/QuantExt/qle/instruments/multilegoption.hpp

for a while now already, in conjunction with numerical integration / finite-difference and MC engines. Only the European case still used QuantLib::Swaption and NonStandardSwaption and the corresponding Black pricing engine. I am now working on adapting the Black engine to MultiLegOption and generalizing it to the remaining coupon types, so that all cases can be uniformly represented by MultiLegOption and our existing numerical engines can also be applied to the European case. SwaptionHelper is still on Swaption and the QL Black engine. We might move to the new Black engine there too.

Long story short, this development here has no high priority for us at the moment. Of course I'd be happy to migrate our approach to QuantLib, but I appreciate that this would be too disruptive (correct me if I am wrong!), so that we'll probably just keep the variants in parallel.

thrasibule · 2024-02-14T19:15:10Z

What do you think of the current PR? This is a natural extension of ##1789. I don't think it breaks anything, and it allows to price OIS european swaptions using the Black engine, which is better than the status quo.

lballabio · 2024-02-15T15:14:00Z

I still have to find some time to look at the new changes, but yes, I'd try to get it into next release.

ql/instruments/makeswaption.cpp

ql/models/shortrate/calibrationhelpers/swaptionhelper.cpp

ql/pricingengines/swaption/blackswaptionengine.hpp

lballabio · 2024-02-20T14:41:44Z

ql/models/shortrate/calibrationhelpers/swaptionhelper.hpp

-                       Real shift = 0.0);
+                       Real shift = 0.0,
+                       Natural settlementDays = Null<Size>(),
+                       RateAveraging::Type averagingMethod = RateAveraging::Compound);


I'm not sure that we want to pass the averaging method yet—the engines we can use with the swaption helper (Jamshidian etc) don't manage OIS yet and are going to treat the swaps as vanilla during calibration. This might be ok if the averaging method is compounding, because of the telescopic property, but not for arithmetic averaging. I wouldn't add the parameter yet and would hard-code the method instead.

ralfkonrad · 2024-02-21T11:47:28Z

I'm not fully convinced by the approach yet.

In SwaptionHelper we start already to distinguish between VanillaSwap and OvernightIndexedSwap. This will in the long run be spread into other parts of the library like engines as well, I guess. The idea of c++ and OOP is to get rid of these if and switch statements by using a clean class hierarchy.
Pricing engine like the TreeSwaptionEngine will probably fail or at least give wrong results if we pass an OIS swap into it. We might make them fail fast by checking for a VanillaSwap but then see remark 1 above.
The G2SwaptionEngine might except an OIS swap but there is no corresponding test case to check the result of these engines. As far as I can see only the BlackSwaptionEngine is currently tested.

I am still in favor of doing it the hard but consistent way to have a class likeOISSwaption with its own engines etc. and avoiding code duplication by using templates and/or an abstract base class like FixedVsFloatingSwaption.

thrasibule · 2024-02-22T03:20:44Z

SwaptionHelper is a little awkward. Maybe it makes sense to have a separate OISSwaptionHelper class, where we can pass the averagingmethod there. Other than that I don't see why we need to create a new instrument class just because a particular engine doesn't give the right results. BlackSwaptionEngine doesn't give the right result with Bermudan exercise for instance.

lballabio · 2024-03-05T13:43:27Z

A separate OISSwaptionHelper class might be a good idea. Existing engines not working with OIS are not a problem but they should probably raise an exception if passed an OIS swaption. What do you think?

lballabio · 2024-03-15T16:06:41Z

I started separating SwaptionHelper and OISSwaptionHelper but when I got halfway I realized that the switch was now isolated in a small method makeSwap. I might stop here. What do you think?

thrasibule · 2024-03-15T16:35:52Z

This definitely less invasive. I've done the whole hierarchy here: thrasibule@038a30b (with almost the same makeSwap method). The only advantage is if we want to pass additional parameters to overnightindexed swaption like averaging method.

lballabio added this to the Release 1.30 milestone Feb 8, 2023

sweemer reviewed Feb 8, 2023

View reviewed changes

lballabio removed this from the Release 1.30 milestone Mar 25, 2023

github-actions bot added the stale label May 25, 2023

lballabio added in progress and removed stale labels May 25, 2023

lballabio mentioned this pull request Sep 18, 2023

Add common fixed-vs-floater base class for vanilla swap and OIS #1789

Merged

try to prevent notifying the swaption

1d05432

thrasibule added 2 commits February 13, 2024 13:06

make constructor explicit

ace449b

move test to Faster section

c5eb7e3