[ML] Fix bug upgrading state from 5.6 direct to 6.3 causing SIGSEGV o…

…n open job (#143)

Closes #141.

docs/CHANGELOG.asciidoc

@@ -74,6 +74,7 @@ Fix corner case failing to calculate lgamma values and the correspoinding log er
 Function description for population lat_long results should be lat_long instead of mean ({pull}81[#81])
 By-fields should respect model_plot_config.terms ({pull}86[#86])
 The trend decomposition state wasn't being correctly upgraded potentially causing the autodetect process to abort ({pull}136[#136])
+Fix a SIGSEGV in the autodetect process when jump upgrading from 5.6 to 6.3 ({pull}143[#143])
 
 === Regressions
 

include/maths/CTimeSeriesDecompositionDetail.h

@@ -374,6 +374,7 @@ class MATHS_EXPORT CTimeSeriesDecompositionDetail {
 
         //! Initialize by reading state from \p traverser.
         bool acceptRestoreTraverser(const SDistributionRestoreParams& params,
+                                    core_t::TTime lastValueTime,
                                     core::CStateRestoreTraverser& traverser);
 
         //! Persist state by passing information to \p inserter.

lib/maths/CSeasonalComponentAdaptiveBucketing.cc

@@ -333,7 +333,11 @@ bool CSeasonalComponentAdaptiveBucketing::acceptRestoreTraverser(core::CStateRes
             RESTORE(LAST_UPDATES_OLD_TAG,
                     core::CPersistUtils::fromString(traverser.value(), lastUpdates))
         } while (traverser.next());
-
+        // This wasn't present in version 5.6 so needs to be default
+        // initialised if it is missing from the state object.
+        if (lastUpdates.empty()) {
+            lastUpdates.resize(regressions.size(), UNSET_TIME);
+        }
         m_Buckets.clear();
         m_Buckets.reserve(regressions.size());
         for (std::size_t i = 0u; i < regressions.size(); ++i) {

lib/maths/CTimeSeriesDecomposition.cc

@@ -134,9 +134,10 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(const SDistributionRestore
             RESTORE(CALENDAR_CYCLIC_TEST_6_3_TAG,
                     traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
                                                            &m_CalendarCyclicTest, _1)))
-            RESTORE(COMPONENTS_6_3_TAG, traverser.traverseSubLevel(boost::bind(
-                                            &CComponents::acceptRestoreTraverser,
-                                            &m_Components, boost::cref(params), _1)))
+            RESTORE(COMPONENTS_6_3_TAG,
+                    traverser.traverseSubLevel(
+                        boost::bind(&CComponents::acceptRestoreTraverser, &m_Components,
+                                    boost::cref(params), m_LastValueTime, _1)))
         }
     } else {
         // There is no version string this is historic state.
@@ -149,9 +150,10 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(const SDistributionRestore
             RESTORE(CALENDAR_CYCLIC_TEST_OLD_TAG,
                     traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
                                                            &m_CalendarCyclicTest, _1)))
-            RESTORE(COMPONENTS_OLD_TAG, traverser.traverseSubLevel(boost::bind(
-                                            &CComponents::acceptRestoreTraverser,
-                                            &m_Components, boost::cref(params), _1)))
+            RESTORE(COMPONENTS_OLD_TAG,
+                    traverser.traverseSubLevel(
+                        boost::bind(&CComponents::acceptRestoreTraverser, &m_Components,
+                                    boost::cref(params), m_LastValueTime, _1)))
         } while (traverser.next());
         this->decayRate(decayRate);
     }

lib/maths/CTimeSeriesDecompositionDetail.cc

@@ -323,23 +323,22 @@ const std::string LAST_UPDATE_OLD_TAG{"j"};
 
 const double MODEL_WEIGHT_UPGRADING_TO_VERSION_6_3{48.0};
 
-bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
-                                   CTrendComponent& trend,
-                                   core::CStateRestoreTraverser& traverser) {
+bool upgradeTrendModelToVersion_6_3(const core_t::TTime bucketLength,
+                                    const core_t::TTime lastValueTime,
+                                    CTrendComponent& trend,
+                                    core::CStateRestoreTraverser& traverser) {
     using TRegression = CRegression::CLeastSquaresOnline<3, double>;
 
     TRegression regression;
     double variance{0.0};
     core_t::TTime origin{0};
-    core_t::TTime lastUpdate{0};
     do {
         const std::string& name{traverser.name()};
         RESTORE(REGRESSION_OLD_TAG,
                 traverser.traverseSubLevel(boost::bind(
                     &TRegression::acceptRestoreTraverser, &regression, _1)))
         RESTORE_BUILT_IN(VARIANCE_OLD_TAG, variance)
         RESTORE_BUILT_IN(TIME_ORIGIN_OLD_TAG, origin)
-        RESTORE_BUILT_IN(LAST_UPDATE_OLD_TAG, lastUpdate)
     } while (traverser.next());
 
     // Generate some samples from the old trend model.
@@ -348,7 +347,8 @@ bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
                   static_cast<double>(bucketLength) / static_cast<double>(4 * WEEK)};
 
     CPRNG::CXorOShiro128Plus rng;
-    for (core_t::TTime time = lastUpdate - 4 * WEEK; time < lastUpdate; time += bucketLength) {
+    for (core_t::TTime time = lastValueTime - 4 * WEEK; time < lastValueTime;
+         time += bucketLength) {
         double time_{static_cast<double>(time - origin) / static_cast<double>(WEEK)};
         double sample{regression.predict(time_) + CSampling::normalSample(rng, 0.0, variance)};
         trend.add(time, sample, weight);
@@ -1010,6 +1010,7 @@ CTimeSeriesDecompositionDetail::CComponents::CComponents(const CComponents& othe
 
 bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(
     const SDistributionRestoreParams& params,
+    core_t::TTime lastValueTime,
     core::CStateRestoreTraverser& traverser) {
     if (traverser.name() == VERSION_6_3_TAG) {
         while (traverser.next()) {
@@ -1059,8 +1060,8 @@ bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(
             RESTORE_SETUP_TEARDOWN(TREND_OLD_TAG,
                                    /**/,
                                    traverser.traverseSubLevel(boost::bind(
-                                       upgradeTrendModelToVersion6p3,
-                                       m_BucketLength, boost::ref(m_Trend), _1)),
+                                       upgradeTrendModelToVersion_6_3, m_BucketLength,
+                                       lastValueTime, boost::ref(m_Trend), _1)),
                                    m_UsingTrendForPrediction = true)
             RESTORE_SETUP_TEARDOWN(
                 SEASONAL_OLD_TAG, m_Seasonal = boost::make_unique<CSeasonal>(),

lib/maths/unittest/CTimeSeriesDecompositionTest.cc

@@ -2074,6 +2074,7 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
         0.1, HALF_HOUR, maths::SDistributionRestoreParams{maths_t::E_ContinuousData, 0.1}};
     std::string empty;
 
+    LOG_DEBUG(<< "**** From 6.2 ****");
     LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
     {
         std::string xml;
@@ -2218,6 +2219,73 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
             decomposition.addPoint(time, 10.0);
         }
     }
+
+    LOG_DEBUG(<< "**** From 5.6 ****");
+    LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
+    {
+        std::string xml;
+        load("testfiles/CTimeSeriesDecomposition.5.6.seasonal.state.xml", xml);
+        LOG_DEBUG(<< "Saved state size = " << xml.size());
+
+        core::CRapidXmlParser parser;
+        CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
+        core::CRapidXmlStateRestoreTraverser traverser(parser);
+
+        maths::CTimeSeriesDecomposition decomposition(params, traverser);
+
+        // Check that the decay rates match and the values and variances
+        // predictions match the values obtained from 6.2.
+
+        CPPUNIT_ASSERT_EQUAL(0.01, decomposition.decayRate());
+
+        double meanValue{decomposition.meanValue(18316800)};
+        double meanVariance{decomposition.meanVariance()};
+        LOG_DEBUG(<< "restored mean value    = " << meanValue);
+        LOG_DEBUG(<< "restored mean variance = " << meanVariance);
+        CPPUNIT_ASSERT_DOUBLES_EQUAL(9.91269, meanValue, 0.005);
+        CPPUNIT_ASSERT_DOUBLES_EQUAL(3.99723, meanVariance, 0.5);
+
+        // Check some basic operations on the upgraded model.
+        decomposition.forecast(60480000, 60480000 + WEEK, HALF_HOUR, 90.0, 1.0,
+                               [](core_t::TTime, const TDouble3Vec&) {});
+        for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
+            decomposition.addPoint(time, 10.0);
+        }
+    }
+
+    LOG_DEBUG(<< "*** Trend and Seasonal Components ***");
+    {
+        std::string xml;
+        load("testfiles/CTimeSeriesDecomposition.5.6.trend_and_seasonal.state.xml", xml);
+        LOG_DEBUG(<< "Saved state size = " << xml.size());
+
+        core::CRapidXmlParser parser;
+        CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
+        core::CRapidXmlStateRestoreTraverser traverser(parser);
+
+        maths::CTimeSeriesDecomposition decomposition(params, traverser);
+
+        // Check that the decay rates match and the values and variances
+        // predictions are close to the values obtained from 6.2. We can't
+        // update the state exactly in this case so the tolerances in this
+        // test are significantly larger.
+
+        CPPUNIT_ASSERT_EQUAL(0.024, decomposition.decayRate());
+
+        double meanValue{decomposition.meanValue(10366200)};
+        double meanVariance{decomposition.meanVariance()};
+        LOG_DEBUG(<< "restored mean value    = " << meanValue);
+        LOG_DEBUG(<< "restored mean variance = " << meanVariance);
+        CPPUNIT_ASSERT_DOUBLES_EQUAL(96.5607, meanValue, 4.0);
+        CPPUNIT_ASSERT_DOUBLES_EQUAL(631.094, meanVariance, 7.0);
+
+        // Check some basic operations on the upgraded model.
+        decomposition.forecast(10366200, 10366200 + WEEK, HALF_HOUR, 90.0, 1.0,
+                               [](core_t::TTime, const TDouble3Vec&) {});
+        for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
+            decomposition.addPoint(time, 10.0);
+        }
+    }
 }
 
 CppUnit::Test* CTimeSeriesDecompositionTest::suite() {