Anomaly likelihood py #457

breznak · 2019-05-08T14:37:04Z

AnomalyLikelihood available from pure-python code (/py/) + tests passing
- this pulls utils.py (MovingAverage) + tests
- nupic.support for unit-tests
some python 2-3 compatibility fixes
removed canpn serialization related stuff (will not work in this repo)

Fixes #455 , related #124 #205

as it's not supported

as not supported

for print('hello Py2, old chap', file=std.err) we need from __future_ import print_function

ctrl-z-9000-times · 2019-05-08T15:01:48Z

Wow, this is a lot of code! This will take some time to review...

ctrl-z-9000-times · 2019-05-08T15:02:43Z

py/src/nupic/algorithms/anomaly_likelihood.py

+    learningPeriod + estimationSamples iterations.
+
+    claLearningPeriod and learningPeriod are specifying the same variable,
+    although claLearningPeriod is a deprecated name for it.


We should go ahead and remove this deprecated thing.

I'd prefer not in this PR, this just pulls the code from python to this repo.

breznak · 2019-05-08T15:13:55Z

This will take some time to review...

Actually I hope this would be an easy review :)

It's basically 1:1 to numenta/nupic's code, which I take for reviewed.
I suggest reviewing commit by commit to see only the changes I did. Rest is pulled from nupic.py branch, which tracks nupic.py repo, which tracks original,old numenta/nupic (with some fixes).

I just checked out the needed files, moved them around to correct locations, and made minimal fixes for py2/3,capnp.

breznak · 2019-05-08T15:16:51Z

./../../../build/Release/distr/src/nupic/support/init.py:46: in
from .configuration import Configuration
E ImportError: No module named configuration

I was wondering why that one is not needed locally.

breznak · 2019-05-08T15:21:09Z

This PR is large for need to bring up support for runing py unittests from nupic. Once it's inplace, porting a new test should be as simple as just copying the file.

ctrl-z-9000-times

Will finish code review when I have time.

ctrl-z-9000-times · 2019-05-08T15:04:10Z

py/src/nupic/algorithms/anomaly_likelihood.py

+  def __init__(self,
+               claLearningPeriod=None,
+               learningPeriod=288,
+               estimationSamples=100,


Arguments learningPeriod & estimationSamples do very similar things, can they be merged?

ctrl-z-9000-times · 2019-05-08T15:04:29Z

py/src/nupic/algorithms/anomaly_likelihood.py

+  else:
+    distributionParams = estimateNormal(dataValues[skipRecords:])
+
+    # HACK ALERT! The HTMPredictionModel currently does not handle constant


ctrl-z-9000-times · 2019-05-08T15:06:39Z

py/src/nupic/algorithms/anomaly_likelihood.py

+USAGE
+++++
+
+There are two ways to use the code: using the


Is this code actually complicated enough to justify two APIs?

ctrl-z-9000-times · 2019-05-08T15:08:44Z

py/src/nupic/algorithms/anomaly_likelihood.py

+
+
+
+def estimateNormal(sampleData, performLowerBoundCheck=True):


I'm pretty sure that numpy/scipy have library functions to do this, which return objects with all sorts of conveniences attached.

First of all, I want to say I really appreciate you do deep code review where you understand the code and question the implementation! 👍

As for many of these comments, I agree, but I don't want to act on them in this PR which just brings 1:1 code from numenta's, which can be considered for sort of a standard.

I'm pretty sure that numpy/scipy have library functions to do this, which return objects with all sorts of conveniences attached.

as for the justified changes. Yes, the code could (should?) be improved. The result of your review should be fixed in nupic.py (and then brought back here?)

Q: do we want to deviate from Numenta's python implementation? (we said (only API-wise?) we'll keep strong compatibility.

Q: the nupic repo is definitely more alive than nupic.core, should we offer fix there? @rhyolight will you accept it?

Q: actually, do we want to (long term) have PY code here, shouldn't we just focus on API compat with numenta/nupic or community/nupic.py? (I think it's fine to merge all py code in this repo)

see numenta/nupic-legacy#3876 -> upstream is abandoning active development of nupic, nupic.core, no changes there.

needed for unit-tests in CI, and probably numenta's use of the tests (supports default config files)

ctrl-z-9000-times · 2019-05-08T18:44:49Z

numenta/nupic's code, which I take for reviewed.

I am deeply suspicious of numenta's code. Their ideas are great, their implementations are in places half baked. However since we have a clear need for this code, and because i dont actually have the time to make this better, i will go easy on this code review and not demand too many changes.

breznak · 2019-05-08T19:20:44Z

I am deeply suspicious of numenta's code. Their ideas are great, their implementations are in places half baked.

I have to agree on this one.
Still, that code is published in peer-reviewed papers. It'll be good to have it as baseline, if we fix bugs or make it better, it'll be a documented and replicable improvement.

I'll open a follow up issue to have a crack at AnomalyLikelihood code review when we have time.

Maybe someday? I think most of the things that the support code does is already part of the standard library, which is preferable. - Logging is in the standard library. Nupic should not implement its own logger. - Debugging utilities, use the built in debugger `pdb` instead of make more tools. - Configuration, is mostly the users problem. Nupic should not be overly concerned about setting up the calling application.

ctrl-z-9000-times

Breznak,

I deleted the support module since it was not really needed. The commit message has details about why I did this.

The rest of the code is okay to merge. At some point we should clean this up, but it's not critical. You can review my changes, and then go ahead and merge this if you'd like.

breznak

👍 👍 removing the nupic.support

breznak · 2019-05-10T13:52:58Z

py/src/nupic/tests/algorithms/anomaly_likelihood_jeff_test.py

@@ -35,7 +35,7 @@
 import unittest

 from nupic.algorithms import anomaly_likelihood as an
-from nupic.support.unittesthelpers.testcasebase import TestCaseBase
+from unittest import TestCase as TestCaseBase


I like this change! Makes the diff much smaller, and I agree we should limit the scope of things we care about.
Thanks 👍

breznak added 8 commits May 8, 2019 15:24

AnomalyLikelihood: copied from python repo

2adb1bc

move anomaly likelihood test to correct location

682d7db

Python AnomalyLikelihood: remove capnp Serializable

5c60fa6

as it's not supported

Python: copy MovingAverage utils.py from python repo

87f8125

move utils.py tests to correct location

ae74b00

utils.py remove capnp serialization

ccd6e31

as not supported

Python: merge nupic.support helpers for running tests

a8f284a

python2to3 compatibility fixes

f3a91ec

for print('hello Py2, old chap', file=std.err) we need from __future_ import print_function

breznak added enhancement New feature or request ready anomaly python not py binding, but merge py code in repo labels May 8, 2019

breznak requested review from dkeeney and ctrl-z-9000-times May 8, 2019 14:37

breznak self-assigned this May 8, 2019

ctrl-z-9000-times reviewed May 8, 2019

View reviewed changes

Python: merge nupic.support.configuration

10031e9

needed for unit-tests in CI, and probably numenta's use of the tests (supports default config files)

This was referenced May 8, 2019

Upstream accepting implementation improvements that pass all tests and do not change API? numenta/nupic-legacy#3876

Closed

Add DateTime encoder from python #458

Closed

ctrl-z-9000-times added 2 commits May 10, 2019 09:32

Merge branch 'master' into anomaly_likelihood_py

e55079b

ctrl-z-9000-times approved these changes May 10, 2019

View reviewed changes

breznak commented May 10, 2019

View reviewed changes

breznak merged commit aa7f235 into master May 10, 2019

breznak deleted the anomaly_likelihood_py branch May 10, 2019 14:53

breznak mentioned this pull request May 13, 2019

review and refactor AnomalyLikelihood code (both c++, python) #469

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anomaly likelihood py #457

Anomaly likelihood py #457

breznak commented May 8, 2019

ctrl-z-9000-times commented May 8, 2019

ctrl-z-9000-times May 8, 2019

breznak May 8, 2019

breznak commented May 8, 2019 •

edited

Loading

breznak commented May 8, 2019

breznak commented May 8, 2019

ctrl-z-9000-times left a comment

ctrl-z-9000-times May 8, 2019

ctrl-z-9000-times May 8, 2019

ctrl-z-9000-times May 8, 2019

ctrl-z-9000-times May 8, 2019

breznak May 8, 2019

breznak May 8, 2019 •

edited

Loading

ctrl-z-9000-times commented May 8, 2019

breznak commented May 8, 2019

ctrl-z-9000-times left a comment

breznak left a comment

breznak May 10, 2019




		def estimateNormal(sampleData, performLowerBoundCheck=True):

Anomaly likelihood py #457

Anomaly likelihood py #457

Conversation

breznak commented May 8, 2019

ctrl-z-9000-times commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak commented May 8, 2019 • edited Loading

breznak commented May 8, 2019

breznak commented May 8, 2019

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak May 8, 2019 • edited Loading

Choose a reason for hiding this comment

ctrl-z-9000-times commented May 8, 2019

breznak commented May 8, 2019

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak commented May 8, 2019 •

edited

Loading

breznak May 8, 2019 •

edited

Loading