Skip to content
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

Provide missing AnomalyLikelihood from python #455

Closed
2 tasks
breznak opened this issue May 8, 2019 · 2 comments · Fixed by #457
Closed
2 tasks

Provide missing AnomalyLikelihood from python #455

breznak opened this issue May 8, 2019 · 2 comments · Fixed by #457
Labels
anomaly Python Binding python not py binding, but merge py code in repo

Comments

@breznak
Copy link
Member

breznak commented May 8, 2019

as our cpp Likelihood is likely broken and untested, #124

  • Python Likelihood would go to py/ where pure python code resides.
  • it should be easy to port as has very few requirements

import collections
import math
import numbers
import numpy
from nupic.serializable import Serializable
from nupic.utils import MovingAverage
try:
import capnp
except ImportError:
capnp = None
if capnp:
from nupic.algorithms.anomaly_likelihood_capnp import AnomalyLikelihoodProto
class AnomalyLikelihood(Serializable):
  • we need either: py bindings for MovingAverage (and port py tests)
  • replace MovingAverage in py likelihood with python code for it. (maybe prefered?)

This will be used for running NAB #205

@breznak breznak added anomaly Python Binding python not py binding, but merge py code in repo labels May 8, 2019
This was referenced May 8, 2019
@ctrl-z-9000-times
Copy link
Collaborator

I have a different (competing) design for this. The TM class should keep an exponential moving average (EMA) of the anomaly, as well as an EMA of the variance (aka standard deviation squared) of the anomaly.

This would introduce a new parameter to the TM for the window size / time-scale of the EMA.

@breznak
Copy link
Member Author

breznak commented May 8, 2019

I have a different (competing) design for this. The TM class should keep an exponential moving average (EMA) of the anomaly, as well as an EMA of the variance (aka standard deviation squared) of the anomaly.

I won't be against if it's just a few lines of code, but I'm not a huge fan of cramming more (unrelated) code to TM. TM's job is to make predictions for T+1. Even that we added TM.anomaly was compromising its logical integrity for convenience.

  • the advantage of your (2 EMAs) solution would be much simpler implementation 👍

  • disadvantage: peer-review: We'll probably still need AnomalyLikelihood for comparisons with Numenta's benchmarks.

  • I'd prefer suggested solution with a virtual float TM::getAnomalyScore() which you could override with the new implementation without needing to add new code to TM.

(but these comments are probably for a PR/issue with your suggested design)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly Python Binding python not py binding, but merge py code in repo
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants