New class BaseEncoder #314

ctrl-z-9000-times · 2019-03-07T03:17:36Z

See issue #291

TODO:

dkeeney · 2019-03-07T13:47:41Z

Decide if we should force all encoders to implement serialization

I opinion is that if an encoder has state that would affect the outcome of a subsequent cycle then that state should be serialized. It could be argued that incoming data is not state because it is not the consequence of previous processing. I would think that most encoders are stateless.

The configuration parameters in the NetworkAPI have been included in serialization for everything although I question if it should because it 'normally' is not the consequence of previous processing...perhaps a topic for a separate discussion.

breznak · 2019-03-07T15:06:40Z

I opinion is that if an encoder has state that would affect the outcome of a subsequent cycle then that state should be serialized.

+1, serialize. For stateless (most?) encoders, this should be trivial. It is convenient that later the whole algoruthm/NetworkAPI network can be serialized.

ctrl-z-9000-times · 2019-03-07T16:25:21Z

OK, I will make it serialize. Another reason to want serialization is that many encoders are initialized using a random seed, which is not stored anywhere.

breznak

This looks good to me, thanks!
Please add the Serializable,
I'm still not sure about the decode(), but that can be added later.

dkeeney · 2019-03-07T21:27:40Z

are initialized using a random seed

That would qualify as a state. So yes, it should serialize.

ctrl-z-9000-times · 2019-03-09T00:04:45Z

I'd like to take the time with this PR to clean up the ScalarEncoder since the encoder API is changing. This will break the ScalarEncoder API Compatibility by:

Removing method getWidth
Removing methodencodeIntoArray
Changing the constructor to accept a parameter-structure.
Merging PeriodicScalarEncoder into ScalarEncoder

dkeeney · 2019-03-09T00:23:32Z

That sound reasonable. ScalarEncoder would be a good place to try out the new base class.

Also reorder sections of code, minor docs change.

I have NOT yet tested this!

Serialization keeps 6-digits, so check 5-digits of accuracy.

ctrl-z-9000-times · 2019-03-11T01:36:59Z

I finished implementing the new API in the ScalarEncoder and debugging it. Now it fails because of the network API & regions, which I thought I updated correctly but are failing their unit tests. I don't really understand how the networkAPI works, and I don't have any experience debugging it. Any help would be appreciated.

breznak

This is a good base for the encoders! 👍
A few more known TODOs and some my comments.
Looks good and hope we can merge this soon

bindings/py/cpp_src/bindings/encoders/py_ScalarEncoder.cpp

src/examples/hotgym/HelloSPTP.cpp

breznak · 2019-03-12T10:09:21Z

src/examples/hotgym/HelloSPTP.cpp

    tEnc.stop();
+    for(auto i = 0u; i < inputSDR.size; ++i) {
+      input[i] = (UInt) inputSDR.getDense()[i];


nit, you could use VectorHelpers::castVectorType<Byte, UInt>(inputSDR.data()) as used on other places of this PR.
PS: I'll be trying to avoid that by templating the SDR_dense_t 's datatype in SDR in another PR.

I think it would be far easier & better to overhaul this example to use SDRs throughout.

Once encoders support SDR, it should go there. So it could be part of this PR.

src/nupic/encoders/BaseEncoder.hpp

src/nupic/encoders/ScalarEncoder.cpp

src/nupic/encoders/ScalarEncoder.hpp

src/nupic/regions/ScalarSensor.hpp

Thank you

dkeeney

I noticed you are creating a fourth extension library for encoders.
Would the .py code be expecting to find the encoders in a separate extension library?
I would think they would be part of the algorithms library. If you think it makes since to do that then ok but you have some more files to change for deployment.

packaging/setup.py
__init__.py
and probably some others.

dkeeney

  * Numenta Platform for Intelligent Computing (NuPIC)
 * Copyright (C) 2018, chhenning
 *               2019, David McDougall
 *
 * This program is free software: you can redistribute it and/or modify

I don't think we can change the copyright. There is a standard for what the copyright should look like. We can add our name after the copyright but we cannot include ourselves in the copyright.

breznak

I'm happy with the fixes 👍
for me good to go, please have a look at what David was saying and we can merge

ctrl-z-9000-times · 2019-03-12T22:45:02Z

Would the .py code be expecting to find the encoders in a separate extension library?

Previously python encoders existed at:
import nupic.encoders
Now they're at:
import nupic.bindings.encoders

files to change for deployment

The encoders are bundled into the algorithms library, even though they have their own python module. As long as algorithms.cpython-34m.so is available, so are the encoders.

ctrl-z-9000-times · 2019-03-12T23:10:38Z

I don't think we can change the copyright.

rhyolight answers this question at the end of this thread: https://discourse.numenta.org/t/what-is-a-community-fork/3112/10

Also: https://discourse.numenta.org/t/a-word-on-licensing/2023

There is an important distinction between copyright and a license to use the copyrighted work. We absolutely can not change the license. This all must be published under the Affero GNU Public License version 3.

However, I own the copyright for all of the things I've written. If someone was paying me to write this code, then as part of our contract they would own the copyright. We can't change the copyright of the existing code, since we did not write it. If we edit existing code then we gain the copyright for the portions which we add.

ctrl-z-9000-times · 2019-03-12T23:15:29Z

BTW: You're welcome to retroactively add your name to the copyright notices of the files you've added or improved :)

dkeeney · 2019-03-12T23:36:21Z

# ----------------------------------------------------------------------
# Numenta Platform for Intelligent Computing (NuPIC)
# Copyright (C) 20XX, YOUR NAME HERE.  
# Copyright (C) 2013, Numenta, Inc.
#

I stand corrected.

I worked for years as a software contractor where our contract specifically say we have NO rights and we cannot put our name on anything. When I signed the contributor agreement I assumed it contained similar wording but I do not remember if I actually read all of the fine print. Apparently a contributor does have some rights. Cool.

ctrl-z-9000-times · 2019-03-13T01:03:48Z

I'd like to merge this PR so that other tasks can move forward (#291 #278 #304, probable merge conflict with #320). It's not 100% done yet. Here is a checklist list of the remaining tasks which I will post to issue #258 Encoders in C++.

Outstanding Tasks for ScalarEncoder:

C++ example usage & unit test for example
Python example usage & unit test for example
Documentation could use more details, in-depth explanations, and notes for practical usage.
Python script to visually plot the inputs & outputs of the encoder
- Extra Credit if it is an interactive program or has a graphical user interface

breznak

Looks good to me! Thanks for a proper base design for encoders, and fitting it with the existing.
I'm good to merge 👍

ctrl-z-9000-times · 2019-03-13T12:20:07Z

I noticed you are creating a fourth extension library for encoders. [...] If you think it makes since to do that then ok but you have some more files to change for deployment.

Oops. I forgot about that change and I misunderstood your comment. I just made one last commit to include the encoders library in the install process. I don't know why this didn't fail in CI before now...

The reason I added a fourth extension library was to make it show up in the python module hierarchy like it does now.

dkeeney

Ok, I think you have all of the parts now. Looks good.

I assume that each encoder will have its own Region implementation for NetworkAPI. Is that correct?

Thanh-Binh · 2019-03-13T16:51:31Z

I think so!

New class BaseEncoder

d9098e1

ctrl-z-9000-times added the encoder label Mar 7, 2019

ctrl-z-9000-times self-assigned this Mar 7, 2019

breznak requested changes Mar 7, 2019

View reviewed changes

Merge branch 'master' into encoder-baseclass

adab625

ctrl-z-9000-times and others added 11 commits March 10, 2019 09:32

BaseEncoder: Serializable interface

02eb663

Also reorder sections of code, minor docs change.

ScalarEncoder: Use STRUCT for parameters, implements BaseEncoder API

780e5eb

I have NOT yet tested this!

ScalarEncoder: python bindings stubs

27e8ea1

ScalarEncoder implements Serializable interface.

37ab3b8

ScalarEncoder: fix region & example to use new API

f8afc4c

Merge branch 'master' into encoder-baseclass

4d14ac1

ScalarEncoder: python bindings & unit tests

0d10f1d

ScalarEncoder: updated C++ unit tests to work with new API.

50ca930

ScalarEncoder: Serialization unit tests, bug fixes.

403a7d5

ScalarEncoder: Bug fix for Clang.

b092684

ScalarEncoder: Fix missing include

e1163fe

ctrl-z-9000-times force-pushed the encoder-baseclass branch from e5b311d to e1163fe Compare March 11, 2019 00:04

ctrl-z-9000-times added 2 commits March 10, 2019 20:18

ScalarEncoder: unit test fix for python2

c738043

ScalarEncoder: C++ unit test fix

405b5f8

Serialization keeps 6-digits, so check 5-digits of accuracy.

ctrl-z-9000-times force-pushed the encoder-baseclass branch from a65b78d to 405b5f8 Compare March 11, 2019 01:33

ctrl-z-9000-times added the help wanted Extra attention is needed label Mar 11, 2019

ctrl-z-9000-times closed this Mar 11, 2019

ctrl-z-9000-times reopened this Mar 11, 2019

ctrl-z-9000-times removed the help wanted Extra attention is needed label Mar 12, 2019

breznak requested changes Mar 12, 2019

View reviewed changes

ctrl-z-9000-times added 2 commits March 12, 2019 08:11

Merge branch 'master' into encoder-baseclass

1c6aaaf

ScalarEncoder: Breznak's Code Review Changes.

70dce49

Thank you

dkeeney reviewed Mar 12, 2019

View reviewed changes

ScalarEncoder: python bindings reorder namespace statements.

f861846

dkeeney reviewed Mar 12, 2019

View reviewed changes

ctrl-z-9000-times and others added 4 commits March 12, 2019 12:16

ScaralEncoder: fix for -Woverloaded-virtual

50706aa

ScalarEncoder: rename parameter active to activeBits

860e4d9

Merge branch 'master' into encoder-baseclass

60ecd89

ScalarEncoder: Python unit test fix

bb5dd75

breznak reviewed Mar 12, 2019

View reviewed changes

ctrl-z-9000-times added 2 commits March 12, 2019 20:36

ScalarEncoder: Python bindings add docstrings.

53be390

Update API_CHANGELOG.md with ScalarEncoder rewrite, PR #314.

83282f7

ctrl-z-9000-times added the ready label Mar 13, 2019

breznak previously approved these changes Mar 13, 2019

View reviewed changes

Encoder python extension module instalation fixes.

34dd0e8

ctrl-z-9000-times dismissed breznak’s stale review via 34dd0e8 March 13, 2019 12:17

dkeeney approved these changes Mar 13, 2019

View reviewed changes

breznak approved these changes Mar 13, 2019

View reviewed changes

breznak merged commit 96d1111 into master Mar 13, 2019

breznak deleted the encoder-baseclass branch March 13, 2019 15:36

breznak mentioned this pull request Mar 13, 2019

c++ encoders dump #291

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New class BaseEncoder #314

New class BaseEncoder #314

ctrl-z-9000-times commented Mar 7, 2019 •

edited

Loading

dkeeney commented Mar 7, 2019

breznak commented Mar 7, 2019

ctrl-z-9000-times commented Mar 7, 2019

breznak left a comment

dkeeney commented Mar 7, 2019

ctrl-z-9000-times commented Mar 9, 2019

dkeeney commented Mar 9, 2019

ctrl-z-9000-times commented Mar 11, 2019

breznak left a comment

breznak Mar 12, 2019

ctrl-z-9000-times Mar 12, 2019

breznak Mar 12, 2019

dkeeney left a comment •

edited

Loading

dkeeney left a comment

breznak left a comment

ctrl-z-9000-times commented Mar 12, 2019

ctrl-z-9000-times commented Mar 12, 2019

ctrl-z-9000-times commented Mar 12, 2019

dkeeney commented Mar 12, 2019

ctrl-z-9000-times commented Mar 13, 2019

breznak left a comment

ctrl-z-9000-times commented Mar 13, 2019

dkeeney left a comment

Thanh-Binh commented Mar 13, 2019

New class BaseEncoder #314

New class BaseEncoder #314

Conversation

ctrl-z-9000-times commented Mar 7, 2019 • edited Loading

dkeeney commented Mar 7, 2019

breznak commented Mar 7, 2019

ctrl-z-9000-times commented Mar 7, 2019

breznak left a comment

Choose a reason for hiding this comment

dkeeney commented Mar 7, 2019

ctrl-z-9000-times commented Mar 9, 2019

dkeeney commented Mar 9, 2019

ctrl-z-9000-times commented Mar 11, 2019

breznak left a comment

Choose a reason for hiding this comment

breznak Mar 12, 2019

Choose a reason for hiding this comment

ctrl-z-9000-times Mar 12, 2019

Choose a reason for hiding this comment

breznak Mar 12, 2019

Choose a reason for hiding this comment

dkeeney left a comment • edited Loading

Choose a reason for hiding this comment

dkeeney left a comment

Choose a reason for hiding this comment

breznak left a comment

Choose a reason for hiding this comment

ctrl-z-9000-times commented Mar 12, 2019

ctrl-z-9000-times commented Mar 12, 2019

ctrl-z-9000-times commented Mar 12, 2019

dkeeney commented Mar 12, 2019

ctrl-z-9000-times commented Mar 13, 2019

breznak left a comment

Choose a reason for hiding this comment

ctrl-z-9000-times commented Mar 13, 2019

dkeeney left a comment

Choose a reason for hiding this comment

Thanh-Binh commented Mar 13, 2019

ctrl-z-9000-times commented Mar 7, 2019 •

edited

Loading

dkeeney left a comment •

edited

Loading