Skip to content

Commit f54d2b7

Browse files
tveaseytveasey
authored
[6.4][ML] Fix bug upgrading state from 5.6 direct to 6.3 causing SIGSEGV on open job (#144)
1 parent 4008a56 commit f54d2b7

8 files changed

+1974
-15
lines changed

‎docs/CHANGELOG.asciidoc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ Fix corner case failing to calculate lgamma values and the correspoinding log er
6262
Function description for population lat_long results should be lat_long instead of mean ({pull}81[#81])
6363
By-fields should respect model_plot_config.terms ({pull}86[#86])
6464
The trend decomposition state wasn't being correctly upgraded potentially causing the autodetect process to abort ({pull}136[#136])
65+
Fix a SIGSEGV in the autodetect process when jump upgrading from 5.6 to 6.3 ({pull}143[#143])
6566

6667
=== Regressions
6768

‎include/maths/CTimeSeriesDecompositionDetail.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@ class MATHS_EXPORT CTimeSeriesDecompositionDetail {
374374

375375
//! Initialize by reading state from \p traverser.
376376
bool acceptRestoreTraverser(const SDistributionRestoreParams& params,
377+
core_t::TTime lastValueTime,
377378
core::CStateRestoreTraverser& traverser);
378379

379380
//! Persist state by passing information to \p inserter.

‎lib/maths/CSeasonalComponentAdaptiveBucketing.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,11 @@ bool CSeasonalComponentAdaptiveBucketing::acceptRestoreTraverser(core::CStateRes
333333
RESTORE(LAST_UPDATES_OLD_TAG,
334334
core::CPersistUtils::fromString(traverser.value(), lastUpdates))
335335
} while (traverser.next());
336-
336+
// This wasn't present in version 5.6 so needs to be default
337+
// initialised if it is missing from the state object.
338+
if (lastUpdates.empty()) {
339+
lastUpdates.resize(regressions.size(), UNSET_TIME);
340+
}
337341
m_Buckets.clear();
338342
m_Buckets.reserve(regressions.size());
339343
for (std::size_t i = 0u; i < regressions.size(); ++i) {

‎lib/maths/CTimeSeriesDecomposition.cc

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,10 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(const SDistributionRestore
134134
RESTORE(CALENDAR_CYCLIC_TEST_6_3_TAG,
135135
traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
136136
&m_CalendarCyclicTest, _1)))
137-
RESTORE(COMPONENTS_6_3_TAG, traverser.traverseSubLevel(boost::bind(
138-
&CComponents::acceptRestoreTraverser,
139-
&m_Components, boost::cref(params), _1)))
137+
RESTORE(COMPONENTS_6_3_TAG,
138+
traverser.traverseSubLevel(
139+
boost::bind(&CComponents::acceptRestoreTraverser, &m_Components,
140+
boost::cref(params), m_LastValueTime, _1)))
140141
}
141142
} else {
142143
// There is no version string this is historic state.
@@ -149,9 +150,10 @@ bool CTimeSeriesDecomposition::acceptRestoreTraverser(const SDistributionRestore
149150
RESTORE(CALENDAR_CYCLIC_TEST_OLD_TAG,
150151
traverser.traverseSubLevel(boost::bind(&CCalendarTest::acceptRestoreTraverser,
151152
&m_CalendarCyclicTest, _1)))
152-
RESTORE(COMPONENTS_OLD_TAG, traverser.traverseSubLevel(boost::bind(
153-
&CComponents::acceptRestoreTraverser,
154-
&m_Components, boost::cref(params), _1)))
153+
RESTORE(COMPONENTS_OLD_TAG,
154+
traverser.traverseSubLevel(
155+
boost::bind(&CComponents::acceptRestoreTraverser, &m_Components,
156+
boost::cref(params), m_LastValueTime, _1)))
155157
} while (traverser.next());
156158
this->decayRate(decayRate);
157159
}

‎lib/maths/CTimeSeriesDecompositionDetail.cc

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -323,23 +323,22 @@ const std::string LAST_UPDATE_OLD_TAG{"j"};
323323

324324
const double MODEL_WEIGHT_UPGRADING_TO_VERSION_6_3{48.0};
325325

326-
bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
327-
CTrendComponent& trend,
328-
core::CStateRestoreTraverser& traverser) {
326+
bool upgradeTrendModelToVersion_6_3(const core_t::TTime bucketLength,
327+
const core_t::TTime lastValueTime,
328+
CTrendComponent& trend,
329+
core::CStateRestoreTraverser& traverser) {
329330
using TRegression = CRegression::CLeastSquaresOnline<3, double>;
330331

331332
TRegression regression;
332333
double variance{0.0};
333334
core_t::TTime origin{0};
334-
core_t::TTime lastUpdate{0};
335335
do {
336336
const std::string& name{traverser.name()};
337337
RESTORE(REGRESSION_OLD_TAG,
338338
traverser.traverseSubLevel(boost::bind(
339339
&TRegression::acceptRestoreTraverser, &regression, _1)))
340340
RESTORE_BUILT_IN(VARIANCE_OLD_TAG, variance)
341341
RESTORE_BUILT_IN(TIME_ORIGIN_OLD_TAG, origin)
342-
RESTORE_BUILT_IN(LAST_UPDATE_OLD_TAG, lastUpdate)
343342
} while (traverser.next());
344343

345344
// Generate some samples from the old trend model.
@@ -348,7 +347,8 @@ bool upgradeTrendModelToVersion6p3(const core_t::TTime bucketLength,
348347
static_cast<double>(bucketLength) / static_cast<double>(4 * WEEK)};
349348

350349
CPRNG::CXorOShiro128Plus rng;
351-
for (core_t::TTime time = lastUpdate - 4 * WEEK; time < lastUpdate; time += bucketLength) {
350+
for (core_t::TTime time = lastValueTime - 4 * WEEK; time < lastValueTime;
351+
time += bucketLength) {
352352
double time_{static_cast<double>(time - origin) / static_cast<double>(WEEK)};
353353
double sample{regression.predict(time_) + CSampling::normalSample(rng, 0.0, variance)};
354354
trend.add(time, sample, weight);
@@ -1010,6 +1010,7 @@ CTimeSeriesDecompositionDetail::CComponents::CComponents(const CComponents& othe
10101010

10111011
bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(
10121012
const SDistributionRestoreParams& params,
1013+
core_t::TTime lastValueTime,
10131014
core::CStateRestoreTraverser& traverser) {
10141015
if (traverser.name() == VERSION_6_3_TAG) {
10151016
while (traverser.next()) {
@@ -1059,8 +1060,8 @@ bool CTimeSeriesDecompositionDetail::CComponents::acceptRestoreTraverser(
10591060
RESTORE_SETUP_TEARDOWN(TREND_OLD_TAG,
10601061
/**/,
10611062
traverser.traverseSubLevel(boost::bind(
1062-
upgradeTrendModelToVersion6p3,
1063-
m_BucketLength, boost::ref(m_Trend), _1)),
1063+
upgradeTrendModelToVersion_6_3, m_BucketLength,
1064+
lastValueTime, boost::ref(m_Trend), _1)),
10641065
m_UsingTrendForPrediction = true)
10651066
RESTORE_SETUP_TEARDOWN(
10661067
SEASONAL_OLD_TAG, m_Seasonal = boost::make_unique<CSeasonal>(),

‎lib/maths/unittest/CTimeSeriesDecompositionTest.cc

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2074,6 +2074,7 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
20742074
0.1, HALF_HOUR, maths::SDistributionRestoreParams{maths_t::E_ContinuousData, 0.1}};
20752075
std::string empty;
20762076

2077+
LOG_DEBUG(<< "**** From 6.2 ****");
20772078
LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
20782079
{
20792080
std::string xml;
@@ -2218,6 +2219,73 @@ void CTimeSeriesDecompositionTest::testUpgrade() {
22182219
decomposition.addPoint(time, 10.0);
22192220
}
22202221
}
2222+
2223+
LOG_DEBUG(<< "**** From 5.6 ****");
2224+
LOG_DEBUG(<< "*** Seasonal and Calendar Components ***");
2225+
{
2226+
std::string xml;
2227+
load("testfiles/CTimeSeriesDecomposition.5.6.seasonal.state.xml", xml);
2228+
LOG_DEBUG(<< "Saved state size = " << xml.size());
2229+
2230+
core::CRapidXmlParser parser;
2231+
CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
2232+
core::CRapidXmlStateRestoreTraverser traverser(parser);
2233+
2234+
maths::CTimeSeriesDecomposition decomposition(params, traverser);
2235+
2236+
// Check that the decay rates match and the values and variances
2237+
// predictions match the values obtained from 6.2.
2238+
2239+
CPPUNIT_ASSERT_EQUAL(0.01, decomposition.decayRate());
2240+
2241+
double meanValue{decomposition.meanValue(18316800)};
2242+
double meanVariance{decomposition.meanVariance()};
2243+
LOG_DEBUG(<< "restored mean value = " << meanValue);
2244+
LOG_DEBUG(<< "restored mean variance = " << meanVariance);
2245+
CPPUNIT_ASSERT_DOUBLES_EQUAL(9.91269, meanValue, 0.005);
2246+
CPPUNIT_ASSERT_DOUBLES_EQUAL(3.99723, meanVariance, 0.5);
2247+
2248+
// Check some basic operations on the upgraded model.
2249+
decomposition.forecast(60480000, 60480000 + WEEK, HALF_HOUR, 90.0, 1.0,
2250+
[](core_t::TTime, const TDouble3Vec&) {});
2251+
for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
2252+
decomposition.addPoint(time, 10.0);
2253+
}
2254+
}
2255+
2256+
LOG_DEBUG(<< "*** Trend and Seasonal Components ***");
2257+
{
2258+
std::string xml;
2259+
load("testfiles/CTimeSeriesDecomposition.5.6.trend_and_seasonal.state.xml", xml);
2260+
LOG_DEBUG(<< "Saved state size = " << xml.size());
2261+
2262+
core::CRapidXmlParser parser;
2263+
CPPUNIT_ASSERT(parser.parseStringIgnoreCdata(xml));
2264+
core::CRapidXmlStateRestoreTraverser traverser(parser);
2265+
2266+
maths::CTimeSeriesDecomposition decomposition(params, traverser);
2267+
2268+
// Check that the decay rates match and the values and variances
2269+
// predictions are close to the values obtained from 6.2. We can't
2270+
// update the state exactly in this case so the tolerances in this
2271+
// test are significantly larger.
2272+
2273+
CPPUNIT_ASSERT_EQUAL(0.024, decomposition.decayRate());
2274+
2275+
double meanValue{decomposition.meanValue(10366200)};
2276+
double meanVariance{decomposition.meanVariance()};
2277+
LOG_DEBUG(<< "restored mean value = " << meanValue);
2278+
LOG_DEBUG(<< "restored mean variance = " << meanVariance);
2279+
CPPUNIT_ASSERT_DOUBLES_EQUAL(96.5607, meanValue, 4.0);
2280+
CPPUNIT_ASSERT_DOUBLES_EQUAL(631.094, meanVariance, 7.0);
2281+
2282+
// Check some basic operations on the upgraded model.
2283+
decomposition.forecast(10366200, 10366200 + WEEK, HALF_HOUR, 90.0, 1.0,
2284+
[](core_t::TTime, const TDouble3Vec&) {});
2285+
for (core_t::TTime time = 60480000; time < 60480000 + WEEK; time += HALF_HOUR) {
2286+
decomposition.addPoint(time, 10.0);
2287+
}
2288+
}
22212289
}
22222290

22232291
CppUnit::Test* CTimeSeriesDecompositionTest::suite() {

0 commit comments

Comments
 (0)