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

Anomaly score calculation separated to own function #739

Merged

Conversation

Zbysekz
Copy link

@Zbysekz Zbysekz commented Oct 31, 2019

I've just moved anomaly score calculation in TemporalMemory::compute function to separate function and enriched py bindings by binding to that function.

I need this, to be able to calculate anomaly score if i am calling activateDendrites and activateCells separately as discussed briefly in #658

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zbysekz code-wise the PR is good, a small change below
I'm somewhat reluctant to this change, as it mixes in Anomaly functionality into TM, which should have stayed separated,

  • TM.anomaly is a convenience and only works (as stated in doc) if you use compute() only in your code
  • for other scenarios you're on your own and have Anomaly, AnomalyLikelihood classes at your service (with full customization)

So the possible outcomes I see:

  • rejecting the PR and using an instance of anomaly yourself (imho clearer design, unless too much extra code?)
  • some compromise and making this function private in c++, and being more benevolent in py and exposing it there.

What do you think about this, conceptually? Or do you see a better way?

src/htm/algorithms/TemporalMemory.hpp Outdated Show resolved Hide resolved
@breznak
Copy link
Member

breznak commented Nov 1, 2019

What I'll definitely like is the separation to a (private) method. The question is about exposing the functionality to outside world.

  • one look is "if it can do that, allow us to use it"
  • other "one class should do one thing well", and therefore is TM, Anomaly.

@Zbysekz
Copy link
Author

Zbysekz commented Nov 1, 2019

Well i just did it this way because it was easier way.

I will modify this function according to your comments & delete pybind.

But you are right, that we shouldn't mix TM and Anomaly.
So how can i use Anomaly ? It seems that Anomaly py bindings is missing, so i should create one?

@breznak
Copy link
Member

breznak commented Nov 1, 2019

So how can i use Anomaly ? It seems that Anomaly py bindings is missing, so i should create one?

for c++ we have in src/htm/algorithms/

in python:

  • TM.anomaly (works OK for "RAW", maybe broken for other modes). But as you say, it's not for a flexible usecase.
  • there is python-only (and working correct) py/htm/algorithms/anomaly_likelihood.py

So:

  • it would definitely be nice to make bindings for Anomaly.hpp 👍
  • maybe also do bindings for the c++ likelihood (duplicates the pure python file), and we'll have to compare/fix the c++ implementation

@breznak
Copy link
Member

breznak commented Nov 1, 2019

it would definitely be nice to make bindings for Anomaly.hpp +1

alternatively, only write the class in pure python, it should be quite easy implementation with numpy or SDR

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as discussed:

  • keep the separate c++ function, but make it private
  • implement Anomaly for python, either as bindings, or as a pure python code
    • and remove the bindings for TM related to anomaly in this PR
  • optional: bindings for Likelihood (already pure python impl exists) + help verifying/fixing the c++ implementation

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one change and this looks good.
Will you want to address the py/bindings for anomaly as a part of this PR too?

src/htm/algorithms/TemporalMemory.hpp Outdated Show resolved Hide resolved
@Zbysekz
Copy link
Author

Zbysekz commented Nov 2, 2019

I will let you know, i think i will create anomaly calculation in pure python - at the end it is just simple calculation. I don't think that for one simple function is the effort of creating binding reasonable.

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on the new Anomaly py implementation, please have a look below

py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
@Zbysekz
Copy link
Author

Zbysekz commented Nov 3, 2019

@breznak I commited this just because i am switching from laptop to desktop PC and i am now being interrupted often (my 2month old son :) ) so this is not yet to be merged, but i take your suggestions

@breznak
Copy link
Member

breznak commented Nov 3, 2019

I commited this just because i am switching from laptop to desktop

ok, seemed to me somehow half-aced.. ;) Enjoy the time with the baby!

@Zbysekz
Copy link
Author

Zbysekz commented Nov 3, 2019

@breznak thanks. Maybe you can help me, suddenly when i compile htm.core i get error "Python.h" no such file or directory .. i know that this is related to missing python-dev (or python3-dev) but i have this installed... and previously it the compilation was successfull, do you have any advice?

[ 44%] Building CXX object bindings/py/cpp_src/CMakeFiles/sdr.dir/bindings/sdr/sdr_module.cpp.o In file included from /media/Data/Data/HTM/htm.core/bindings/py/cpp_src/bindings/sdr/sdr_module.cpp:24:0: /media/Data/Data/HTM/htm.core/bindings/py/cpp_src/bindings/suppress_register.hpp:35:10: fatal error: Python.h: No such file or directory #include <Python.h> ^~~~~~~~~~ compilation terminated.

@breznak
Copy link
Member

breznak commented Nov 3, 2019

i know that this is related to missing python-dev (or python3-dev) but i have this installed...

try removing the build/ folder and starting afresh? I'm also using venv to ensure the pythons dont mix,...

@breznak breznak added the python not py binding, but merge py code in repo label Nov 6, 2019
@Zbysekz
Copy link
Author

Zbysekz commented Nov 7, 2019

ready to merge

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, a few changes below, the biggest concern is

  • does not work for multi-dimensional TM/SP,

py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
py/htm/algorithms/anomaly.py Outdated Show resolved Hide resolved
@Zbysekz
Copy link
Author

Zbysekz commented Nov 8, 2019

Ok, now should be like you suggested

Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look good now, thank you!

Please verify the 2 remaining concerns below and we're good to merge :shipit:

py/tests/algorithms/anomaly.py Outdated Show resolved Hide resolved
Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements, let's merge this!
Thank you for this contribution, @Zbysekz 👍

@breznak breznak merged commit f6d97c9 into htm-community:master Nov 11, 2019
@Zbysekz Zbysekz deleted the temporalMemory_anomalyScoreSeparated branch July 19, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly in_progress python not py binding, but merge py code in repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants