[ML] Move model test helper functions to base class #1523
Conversation
Move existing model test helper functions to base class and share functionality where possible.
|
|
||
| namespace { | ||
|
|
||
| const std::string EMPTY_STRING; |
There was a problem hiding this comment.
This is in an unnamed namespace in several of the files. Could it be made a public or protected static constant on the test fixture? Then it would still be usable in the tests without qualification, but wouldn't be duplicated.
tveasey
left a comment
tveasey
left a comment
There was a problem hiding this comment.
Overall looks good. I made a few minor comments and also wonder if we can pull one more bit of functionality up to the base class.
| CEventData makeEventData(core_t::TTime time, | ||
| std::size_t pid, | ||
| double value, | ||
| const TOptionalStr& influence = TOptionalStr()) { |
There was a problem hiding this comment.
I think it should be possible to move this into the base class. We have makeEventData in CEventRateModelTest which I think this could be combined with.
| << ml::core::CContainerPrinter::print(orderedAnomalies)); | ||
| } | ||
|
|
||
| void CModelTestFixtureBase::generateAndCompareKey(const ml::model::function_t::TFunctionVec& countFunctions, |
There was a problem hiding this comment.
This seems like it should be static.
|
|
||
| int detectorIndex{0}; | ||
| for (const auto& countFunction : countFunctions) { | ||
| for (bool usingNull : useNull) { |
There was a problem hiding this comment.
Probably just moved here, but this can be more concise:
| for (bool usingNull : useNull) { | |
| for (bool usingNull : {true, false}) { |
then delete useNull. Similar for others below.
| for (std::size_t i = 0u; i < values.size(); ++i) { | ||
| processBucket(time, bucketLength, values[i], influencers[i], *gatherer, | ||
| m_ResourceMonitor, model, annotatedProbability); | ||
| processBucket(time, bucketLength, values[i], influencers[i], |
There was a problem hiding this comment.
For consistency it feels like all the calls to processBucket should be qualified (it's our usual style and you've done it for the other methods you've added):
| processBucket(time, bucketLength, values[i], influencers[i], | |
| this->processBucket(time, bucketLength, values[i], influencers[i], |
Move existing model test helper functions to base class and share functionality where possible.
3 participants
Move existing model test helper functions to base class and share functionality where possible.
While this goes some way to reducing code duplication there may still be scope to
Relates to #1477