Repository navigation
[ML] Fix restore from checkpoint damaging seasonality modelling for anomaly detection - #821
Conversation
droberts195
left a comment
droberts195
left a comment
There was a problem hiding this comment.
Thanks for fixing this. All my comments are minor nits and non-essential things that might help to avoid future mistakes.
| // DO NOT change the existing tags if new sub-classes are added. | ||
| const core::TPersistenceTag DIURNAL_TIME_TAG{"a", "diurnal_time"}; | ||
| const core::TPersistenceTag ARBITRARY_PERIOD_TIME_TAG{"b", "arbitrary_period_time"}; | ||
| const std::string DELIMITER(","); |
There was a problem hiding this comment.
There's a subtle dependency between this and the DELIMITER and PAIR_DELIMITER in core::CPersistUtils. This one has to be different to those two, since restoration is searching for it starting at the beginning of a string whose beginning portion contains core::CPersistUtils::DELIMITER and core::CPersistUtils::PAIR_DELIMITER. So this could be commented. Alternatively rfind could be used instead of find when deconstructing the persisted string.
There was a problem hiding this comment.
I like the rfind idea. I thought about defining both here to make this explicit, but decided this unit test would fail if anyone changed the delimiter used by CPersistUtils. (On that front, this would also invalidate all our saved state so I don't see it ever happening, but better to be safe.)
| #include <maths/CChecksum.h> | ||
| #include <maths/CIntegerTools.h> | ||
|
|
||
| #include <boost/array.hpp> |
There was a problem hiding this comment.
It looks like this file had already been changed to std::array on master without changing the header, so this is fine on master. But note that on 6.8 the code is still using boost::array, so the header change shouldn't be included in the 6.8 backport.
There was a problem hiding this comment.
Yes, I need to rewrite the tests as well on 6.8 so the back port is non-trivial.
|
retest |
…nomaly detection (elastic#821)
…nomaly detection (elastic#821)
…nomaly detection Backport elastic#821.
We weren't persisting and restoring all the state we should have been. This should only affect behaviour after a restore.
Fixes #820.