SDRClassifier: fix precision by using Real64 for PDF #667

breznak · 2019-09-16T16:50:56Z

without this, the scores never correctly converge(learn).
Thanks @Thanh-Binh for finding and solving this bug.

Additionally, add some docs to the class.

Fixes #646

TODO:

We need a test to replicate the reported issue before this is merged

@Thanh-Binh

without this, the scores never correctly converge(learn). Thanks @Thanh-Binh for finding and solving this bug. Additionally, add some docs to the class.

use Real64 for weights_ too, make some methods const

as inference should never change internal state,

this removes updateHistory_() from inference, updateHistory code was moved directly into learn() and rest split to a new method checkMonotonic_()

breznak

use Real64 for weights and PDF
newly makes infer() const

breznak · 2019-09-16T20:35:41Z

src/htm/algorithms/SDRClassifier.cpp

-{
-  updateHistory_( recordNum, pattern );
+Predictions Predictor::infer(const UInt recordNum, const SDR &pattern) const {
+  checkMonotonic_(recordNum);


@ctrl-z-9000-times please have a look on the last 1,(2) commits, I intended to make infer const, as it imho should have been. For Classifier that was easy, for Predictor I had to remove the updateHistory_ from infer (no tests visibly broken).

Why does it matter if infer is constant? I doubt it will have any performance impact, and I don't think this will prevent any programming mistakes.

Consider the following chain of events:

infer( t, SDR-A ) -> PDF learn( t+1, SDR-B, Labels )

The method updateHistory stores the given SDR inside of the predictor. Previously the call to learn would have associated SDR-A with Labels since that SDR was given to infer with. Now that will not happen.

Also, these changes allow the timestamps to the infer method to go backwards.

infer( t + 2, ... ) infer( t, ... )

I'm not saying the new behavior is wrong, just that it changed.

I doubt it will have any performance impact, and I don't think this will prevent any programming mistakes.

right, there won't be any performance gains, and the behavior has changed. I think it adds (expected) semantics, and better separates responsibilities of those 2 functions:

you know only what you learn()

and you can infer() const anytime without any worry of changing the state (comes with loosened requirement for monotonic timestamps)

Ok, that makes sense +1

src/htm/algorithms/SDRClassifier.hpp

breznak · 2019-09-18T06:26:02Z

@Thanh-Binh Can you please test that this branch fixes the problem with Predictor you describe in #646 ?

Also, how is going your work on publishing the reproducible example for this issue? In the meanwhile, I'll try to add Predictor to the hotgym example (that also uses sine waves).

Thanh-Binh · 2019-09-18T17:44:07Z

@breznak
I think the crash problem comes from the fact that you remove
updateHistory_( recordNum, pattern );
in the function infer().

breznak · 2019-09-18T18:28:55Z

I think the crash problem comes from the fact that you remove
updateHistory_( recordNum, pattern );

the strange thing is debug mode (and gdb) is not able to detect/crash, that's a strange issue.
I can look into this.

A different solution would be removing recordNum altogether. I find it unneeded for the inference (maybe also learn) where I want to set a SDR and predict N-th next pattern/label. #675 (comment)

EDIT: and actually no, this PR (removes updateHistory) does not crash, while #675 does (with update h)

as numRecord is no longer needed for inference. API changes to bidings and tests to reflect the change. Several fixes from earlier commits in this PR, this fixes the segfaults.

breznak

fixes
refactored infer() to drop recordNum argument

This is now ready for your reviews.

breznak · 2019-09-19T23:30:21Z

src/htm/algorithms/SDRClassifier.cpp

  }
  return result;
 }


-void Predictor::learn(const UInt recordNum, const SDR &pattern,
+void Predictor::learn(const UInt recordNum, //TODO make recordNum optional, autoincrement as steps 


for sequential use, even in learn() recordNum could be made optional

Yea, recordNum is only useful if the user wants to skip time steps, which is not the common case. However when working with time series data sets, there are valid situations where you don't want to learn about a specific time-step so we should keep the time-skip capability in some form.

ctrl-z-9000-times

This looks fine to me.

breznak · 2019-09-20T07:13:45Z

Thanks for reviewing, followup TODOs are:

Predictor.learn(): make arg recordNum optional
fix crash with too large category, seen in Hotgym predictor, anomaly tests #675

Thanh-Binh · 2019-09-20T09:23:42Z

@breznak I do not think if remove updateHistory() from infer() is good idea, because it is thought for dynamically changing the learn and inference mode in the real-time.

breznak · 2019-09-20T09:31:51Z

if remove updateHistory() from infer() is good idea, because it is thought for dynamically changing the learn and inference mode in the real-time.

I don't understand, can you explain, please?
See #667 (comment) for why I think the new behavior is more correct. You can call learn/infer at anytime, recordNum has nothing to do with it. Do you have a usecase where the current functionality is insufficient?

breznak added 4 commits September 16, 2019 18:48

SDRClassifier: fix precision by using Real64 for PDF

ccf7e10

without this, the scores never correctly converge(learn). Thanks @Thanh-Binh for finding and solving this bug. Additionally, add some docs to the class.

Classifier: more fixes

d437a6a

use Real64 for weights_ too, make some methods const

Classifier: make infer const

d34d0cd

as inference should never change internal state,

Predictor: make infer const

bf5123b

this removes updateHistory_() from inference, updateHistory code was moved directly into learn() and rest split to a new method checkMonotonic_()

breznak requested review from ctrl-z-9000-times and dkeeney September 16, 2019 19:32

breznak commented Sep 16, 2019

View reviewed changes

ctrl-z-9000-times reviewed Sep 17, 2019

View reviewed changes

src/htm/algorithms/SDRClassifier.hpp Outdated Show resolved Hide resolved

breznak added 3 commits September 17, 2019 19:00

Merge branch 'master_community' into predictor_precision_fix

2609617

review: formatting

22d6664

Merge branch 'master_community' into predictor_precision_fix

4fd8fbb

breznak mentioned this pull request Sep 18, 2019

Hotgym predictor, anomaly tests #675

Open

2 tasks

breznak self-assigned this Sep 18, 2019

breznak added the ready label Sep 18, 2019

breznak requested a review from ctrl-z-9000-times September 18, 2019 14:08

breznak added 4 commits September 19, 2019 15:54

Merge branch 'master_community' into predictor_precision_fix

767f89a

Predictor: use hashmap

2aedac2

Classifier: simplify asserts

4d8708d

Predictor: remove numRecord arg, fixes

df7ae4e

as numRecord is no longer needed for inference. API changes to bidings and tests to reflect the change. Several fixes from earlier commits in this PR, this fixes the segfaults.

breznak commented Sep 19, 2019

View reviewed changes

ctrl-z-9000-times approved these changes Sep 20, 2019

View reviewed changes

breznak merged commit 1565a50 into master Sep 20, 2019

breznak deleted the predictor_precision_fix branch September 20, 2019 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDRClassifier: fix precision by using Real64 for PDF #667

SDRClassifier: fix precision by using Real64 for PDF #667

breznak commented Sep 16, 2019 •

edited

Loading

breznak left a comment

breznak Sep 16, 2019

ctrl-z-9000-times Sep 17, 2019

breznak Sep 17, 2019

ctrl-z-9000-times Sep 17, 2019

breznak commented Sep 18, 2019

Thanh-Binh commented Sep 18, 2019

breznak commented Sep 18, 2019 •

edited

Loading

breznak left a comment

breznak Sep 19, 2019

ctrl-z-9000-times Sep 20, 2019

ctrl-z-9000-times left a comment •

edited

Loading

breznak commented Sep 20, 2019

Thanh-Binh commented Sep 20, 2019

breznak commented Sep 20, 2019

SDRClassifier: fix precision by using Real64 for PDF #667

SDRClassifier: fix precision by using Real64 for PDF #667

Conversation

breznak commented Sep 16, 2019 • edited Loading

breznak left a comment

Choose a reason for hiding this comment

breznak Sep 16, 2019

Choose a reason for hiding this comment

ctrl-z-9000-times Sep 17, 2019

Choose a reason for hiding this comment

breznak Sep 17, 2019

Choose a reason for hiding this comment

ctrl-z-9000-times Sep 17, 2019

Choose a reason for hiding this comment

breznak commented Sep 18, 2019

Thanh-Binh commented Sep 18, 2019

breznak commented Sep 18, 2019 • edited Loading

breznak left a comment

Choose a reason for hiding this comment

breznak Sep 19, 2019

Choose a reason for hiding this comment

ctrl-z-9000-times Sep 20, 2019

Choose a reason for hiding this comment

ctrl-z-9000-times left a comment • edited Loading

Choose a reason for hiding this comment

breznak commented Sep 20, 2019

Thanh-Binh commented Sep 20, 2019

breznak commented Sep 20, 2019

breznak commented Sep 16, 2019 •

edited

Loading

breznak commented Sep 18, 2019 •

edited

Loading

ctrl-z-9000-times left a comment •

edited

Loading