-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Linear scaling change detection #25
[ML] Linear scaling change detection #25
Conversation
…ge-modelling-part-3
…ge-modelling-part-3
…ents to change detection
…ge-modelling-part-3
…functionality in change detection
…ge-modelling-part-4
|
||
change = candidates[0].second - m_ChangeModels.begin(); | ||
|
||
return p / 0.03125; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this magic value has some origin/explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 1/2^5 and ensures that the value of the function equals one, and we'd just accept the change, at the point at which all the decision variables are at the centre of their respective sigmoid functions and the time is at the lower end of the decision interval. If you think about this as converting the hard decision criteria to a soft one then this means the decision is (just) the same as it would be in the case that all the individual hard criteria are met. I'll add a comment.
|
||
double CUnivariateLinearScaleModel::bic() const | ||
{ | ||
return -2.0 * this->logLikelihood() + std::log(m_SampleCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use fastlog, same below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite possibly. This won't be a bottleneck, but I agree we probably don't need the accuracy here and it is good to use it whenever possible.
@@ -374,9 +431,9 @@ class MATHS_EXPORT CUnivariateTimeShiftModel final : public CUnivariateChangeMod | |||
|
|||
//! Update with \p samples. | |||
virtual void addSamples(std::size_t count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit as you change it: could be const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
std::size_t dimension, | ||
double derate, | ||
double scale, | ||
const core::CSmallVector<double, 10> &value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a typedef/alias here - TDouble10Vec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to introduce a new typedef into the maths namespace just in this header: the reason I try to avoid this is that I think it tends to promote fragile transitive dependencies between headers when something compiles only because it (in)directly includes a header. That said, I think it would probably be worthwhile defining some widely used typedefs such as TDoubleVec
, TDouble1Vec
, etc in a global forward decls header and removing the corresponding duplicate typedefs which we currently have. I'll look at making this change in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some questions
This implements detection of linear scaling events. It also finishes up the unit testing of change detection and fixes some issues these turned up: specifically, 1) the behaviour when a change is detected but the trend model has no components, 2) the handling of time shifts in the trend model and 3) the handling of data types in the trend component change model. Finally, we are now more careful with the weights we apply to samples added to both the standard and change models. This has meant I've been able to revert scaling the changes, since the trend is less influenced by values during the change detection period if we're likely to detect a change.
Removing version label as this is a feature branch PR and it causes confusion when generating release notes. (For interest this was eventually merged to 6.4 and above in #92.) |
This implements detection of linear scaling events. It also finishes up the unit testing of change detection and fixes some issues these turned up: specifically, 1) the behaviour when a change is detected but the trend model has no components, 2) the handling of time shifts in the trend model and 3) the handling of data types in the trend component change model. Finally, we are now more careful with the weights we apply to samples added to both the standard and change models. This has meant I've been able to revert scaling the changes, since the trend is less influenced by values during the change detection period if we're likely to detect a change.