-
Notifications
You must be signed in to change notification settings - Fork 75
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
TM.anomaly: fix score for likelihood anomaly modes #666
base: master
Are you sure you want to change the base?
Conversation
I'd like to include the sine wave experiments before this is merged. |
we returned likelihoods, but TM.anomaly should return anomaly scores, thus we need to return 1.0-likelihood Thanks @Thanh-Binh for reporting this issue!
Please review, @Thanh-Binh does this resolve the issue in your tests? |
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 see no problem.
Thanks for the expanded explanations.
Thanks for reviewing! |
@breznak for log-likelihood, I think your proposal
is not correct. I believe it should be :
|
this is one thing I'm considering: if we want to log anomaly (= 1-like) or the likelihoods (?) does
|
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'm calling hold-off on this PR, until this is properly explained and tested.
@@ -506,7 +506,8 @@ void TemporalMemory::compute(const SDR &activeColumns, | |||
const Real raw = computeRawAnomalyScore( | |||
activeColumns, | |||
cellsToColumns( getPredictiveCells() )); | |||
tmAnomaly_.anomaly_ = tmAnomaly_.anomalyLikelihood_.anomalyProbability(raw); | |||
tmAnomaly_.anomaly_ = 1.0f - tmAnomaly_.anomalyLikelihood_.anomalyProbability(raw); // AnomalyLikelihood returs a likelihood of the score (given past scores), | |||
// and we want to detect unlikely scores, aka big changes -> 1.0-likelihood represents "anomaly" |
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.
actually, I'm not sure this is correct at all.
Revisitiing anomaly likelihood in original Numenta's py code:
def anomalyProbability(self, value, anomalyScore, timestamp=None):
"""
Compute the probability that the current value plus anomaly score represents
an anomaly given the historical distribution of anomaly scores. The closer
the number is to 1, the higher the chance it is an anomaly.
...
And both codes (Numenta's py, and ours cpp) internally toggle likelihoods to anomaly before returning
likelihood = 1.0 - likelihoods[0]
, so doing another 1-anomalyProbability()
switches back to likelihoods[0]
So, according to docs and logic of the code, the TM.anomaly should represent anomalyProbability()
in AnomalyLikelihood.
@Thanh-Binh can you describe in detail your experiment and where you see errors? We must replicate it here, ideally even simpler, synthetic test for likelihood: anomaly scores high 1.0, 0.9, 0.98, 1.0, ... -> anomalyProbability()
should drop. Same for low score: 0.0, 0.1, 0.05, 0.0,.. -> anProb() drops to 0.
For a sudden change (high anomalies -> low, and vice versa) the anProb() should be high.
we used to return likelihood (of the score), but for anomaly we must return
1 - likelihood
Fixes #665
TODO: