Fix OREAnalytics Test Suite

[francisduffy]

There were two issues with OREAnalytics test suite:

1. the */ObservationModeTest/testDefer test was crashing due to dereferencing a null pointer.
2. when the test suite is run without */ObservationModeTest/testDefer, there were failures due to the zero coupon and year on year inflation curves.

testDefer Issue

The issue with testDefer was coming from the zero coupon and year on year inflation curve helpers at

Engine/OREAnalytics/test/testmarket.cpp

Line 581 in 1c7fb03

QuantLib::ext::shared_ptr<BootstrapHelper<ZeroInflationTermStructure>> anInstrument(new ZeroCouponInflationSwapHelper(

and

Engine/OREAnalytics/test/testmarket.cpp

Line 610 in 1c7fb03

QuantLib::ext::shared_ptr<BootstrapHelper<YoYInflationTermStructure>> anInstrument(new YearOnYearInflationSwapHelper(

respectively.

The ZeroCouponInflationSwapHelper is not provided an explicit start date in its constructor call. Therefore, updateDates_ in RelativeDateBootstrapHelper is not set to false and hence the ZeroCouponInflationSwapHelper instances observe the Settings::instance().evaluationDate(). The start date was added in commit 721f3c4. The explicit unregisterWith(Settings::instance().evaluationDate()) call could be removed as a consequence.

Similarly for the YearOnYearInflationSwapHelper. However, QuantLib needs to be updated here - it does not set updateDates_ to false in RelativeDateBootstrapHelper when an explicit start date is provided in the constructor. Therefore you still need to unregister with Settings::instance().evaluationDate() manually. This is done as part of another commit, ede6ce1.

Without these changes, updateDates_ in RelativeDateBootstrapHelper is true and when Settings::instance().evaluationDate() changes a call is made to initializeDates() here. This leads to zciis_ in the ZeroCouponInflationSwapHelper and yyiis_ in YearOnYearInflationSwapHelper getting reassigned to new values and the original pointees are destroyed. But, the original pointees are stored in deferredObservers_ here. An attempt is made to call update() on the null pointer here and the test crashes.

Inflation Curve Issues

Zero Tenor Quote Removal

The commit 56c5bfe removes the inflation curve quote with tenor 0. This was ignored in the zero coupon inflation curve bootstrap but caused the year on year inflation curve helpers to fail with errors on schedule creation i.e. errors like effective date (December 15th, 2016) later than or equal to termination date (December 15th, 2016).

Dodgson-Kainth Issue

There is an error on Dodgson-Kainth inflation scenario generation. A fix for the test suite is added in commit b806d85.

Before this fix the zero inflation term structure, due to a series of notifications, was being forced to recalculate on the first simulation date, t_1 say, for every sample and this recalculated curve was being used in the generation of the CPI index scenario values rather than the t_0 calculated curve. The reason for this is outlined below.

On sample 0, there is a call to ScenarioSimMarket::updateScenario(const Date& d) which changes the evaluationDate to the first grid date t_1. The inflation index in the ZCIIS zciis_ inside the helper is observing this evaluationDate and the inflation curve is observing the inflation index. Due to this, the inflation curve is forced to recalculate on the first grid date t_1 for sample 0. This t_1 recalculated curve is used for the generation of all scenarios for t_2,...,t_N for sample 0. Note that if relative to date t_1, some of the inflation helpers in the curve require a CPI fixing, e.g. t_1 >= fixing date of some helper, the test fails with exception from iterative bootstrap about missing fixing. In other words, we see errors of the form QuantLib::Error: 1st iteration: failed at 1st alive instrument, pillar May 1st, 2016, maturity May 1st, 2016, reference date May 1st, 2015: Missing UK RPI fixing for May 1st, 2016.

On samples > 0, the applyScenario(scenario); in ScenarioSimMarket::updateScenario(const Date& d) triggers a notification chain that leads to the inflation curve being recalculated on t_1 for every sample. In summary, the chain is:

• in ScenarioSimMarket's simData_, setValue is called on CPI index quote
• InflationIndexObserver in update() calls setFixing()
• adds fixings in IndexManager which in turn calls notifier(name)->notifyObservers();
• the indices in the ZCIIS swap instruments, i.e. zciis_, in the inflation curve helpers are observing the notifier => they get notified
• the inflation curve is observing the indices in the previous bullet => the curve gets notified and in its update() call sets
  calculated_ to false => recalculation on t_1 for next sample.

The changes above only fix the test suite. For example, you will see the incorrect behaviour if you run example Examples/Exposure/Input/ore_inflation_dk.xml. Would need to discuss further if similar changes i.e.

anInstrument->unregisterWithAll();
anInstrument->registerWith(yts);
anInstrument->registerWith(quote);

for all instruments should be applied at

Engine/OREData/ored/marketdata/inflationcurve.cpp

Line 284 in 1c7fb03

instrument->unregisterWith(Settings::instance().evaluationDate());

and

Engine/OREData/ored/marketdata/inflationcurve.cpp

Line 380 in 1c7fb03

instrument->unregisterWith(Settings::instance().evaluationDate());

The only question I guess is are there any analytics where we rely on the inflation curves reacting to inflation index values being added for the inflation index referenced in the curve's helpers. If not, I think we can safely apply the change above there. If yes, then we need to leave it configurable.

[francisduffy]

However, QuantLib needs to be updated here - it does not set updateDates_ to false in RelativeDateBootstrapHelper when an explicit start date is provided in the constructor.

On this I see that it is already done in the main QuantLib at https://github.com/lballabio/QuantLib/blob/1daa40d013ff700292f421ec9473f419893b1379/ql/termstructures/inflation/inflationhelpers.cpp#L186 but not in the ORE engine fork of QuantLib. I guess I can just add it to ORE engine version.

[pcaspers]

Thank you, @francisduffy.

The only question I guess is are there any analytics where we rely on the inflation curves reacting to inflation index values being added for the inflation index referenced in the curve's helpers. If not, I think we can safely apply the change above there. If yes, then we need to leave it configurable.

I think we can (and should) apply these changes: The objects created in TodaysMarket are not supposed to react to observables, in fact they should not, for this kind of behavior, the ScenarioSimMarket should be used.

As for the testDefer issue: I have not checked this in more detail, but I wonder if this might be a more fundamental issue: When zciis_ gets reassigned to a new value and the original pointee is destroyed, shouldn't it be removed from the set of deferred observers via the call to unregisterObserver() in its destructor here and here?

[francisduffy]

Thanks @pcaspers. I pushed the changes for the objects created in TodaysMarket.

On the testDefer issue, I think it is a fundamental issue in that elements of the deferredObservers_ set can become null during the ObservableSettings::enableUpdates() call. The problem is that at the start of that method here, updatesDeferred_ is set to false. Then, there is the loop over deferredObservers_ calling deferredObserver->update() on each. It is during this loop, that the call to update() for a deferredObserver "earlier" in the deferredObservers_ set (in the case of the test, the RPI index) leads to the destruction of a deferredObserver "later" in the deferredObservers_ set (in the case of the test, the yyiis_ helper). The destructor of the later deferredObserver is called and as you mention Observer::~Observer() and observable->unregisterObserver(this) are called. The issue is that updatesDeferred_ is at this point false and hence the line here in Observable::unregisterObserver is skipped and the deferredObserver that is destructed is left in the deferredObservers_ set.

I can't think of a quick fix other than a (simpler) version of what is done when QL_ENABLE_THREAD_SAFE_OBSERVER_PATTERN is defined. In other words, derive Observer from ext::enable_shared_from_this<Observer> and have deferredObservers_ store a set of weak_ptrs. Then in the loop, the weak pointer could be checked and update called only if still valid else remove the deferred observer from the set. I could give it a try to see if it makes sense. Maybe you can see an easier fix though.

[pcaspers]

It does make sense.

Then in the loop, the weak pointer could be checked and update called only if still valid else remove the deferred observer from the set.

We probably do not need to remove an invalid deferred observer from the set, since the whole set is cleared anyways after the update calls.

The disadvantages of enable_shared_from_this I can see are

• it forces us to use shared pointers everywhere
• it introduces an additional member variable for each observer

If we want to avoid this, we could go for a more manual approach by introducing another flag runningDeferredUpdates_ in ObservableSettings which we set to true at the start of ObservableSettings::enableUpdates(). We can then store the position of unregistered observers in a new member std::set<std::size_t> ObservableSettings::invalidatedObservers within ObservableSettings::unregisterDeferredObserver() instead of erasing them from deferredObservers_ and use this information to skip the invalid positions during the update in enableUpdates(). Maybe there are better variations of this approach. In any case, we do not have to touch Observer and do not introduce additional overhead if deferred observers are not used.

Either way, we should probably try to get this back into Luigi's QuantLib, he will probably have additional thoughts on this.

[francisduffy]

Thanks again. Your proposed fix makes sense to me. I have opened a pull request at lballabio/QuantLib#2409. Feel free to propose changes there if I misinterpreted your proposal. Instead of std::set<std::size_t> to mark the invalid observers, I used ObservableSettings::set_type i.e. std::set<Observer*>. I can add it to the ORE engine fork when / if the PR is accepted.

[pcaspers]

Sounds good.

As you say, the set of invalid observers is probably small in general, therefore using 'find' to identify an invalid observer should be fine. Since we are using two sorted sets we could optimize the update loop a bit if we want, from O(n log n) to O(n) but that's probably overkill?

[francisduffy]

I took another quick look and added an alternative approach in the PR here. I think it is cleaner than having the two sets if we are ok with the memory overhead of storing a bool for every deferred observer.

[pcaspers]

This looks good and is O(n). I think we can live with the added memory overhead.