Resolve sparse field in Spec

[breznak]

Objective:
Discuss removig bool sparse from engine/Spec.hpp;
and possible replacement with defining NTA_BasicType_Sparse
to comply with OPF (py API)

discussed in #136

from @dkeeney 's comments: #136 (comment)

I have a problem. I noticed that a new element 'sparse' was added to the InputSpec and OutputSpec of the Spec class. This breaks all Region Implementations, both C++ and Python. If you look at any Region implementation...for example the TestNode.cpp you will see that there is a static createSpec() function. By design, all plug-ins, C++ or Python, contain this function and it must return a static structure initialized from static constants. This structure tells the Network Infrastructure what the plugin is expecting for parameters, inputs and outputs. By adding this 'sparse' field, the static initializers no longer match the structure so none of them initializes correctly.
A much better way to do this is to define a new NTA_BasicType and call it NTA_BasicType_Sparse or something. And while you are in there you can also add NTA_BasicType_SCR. With those in place, we can modify the those Region implementations that are programmed to accept a sparse array as one of their inputs and specify this new value in its spec. You should also modify the ArrayBase class so that it knows what to do with this new data type. And there are some element size arrays that need to be setup as well to support it.
Now...the current implementation does not allow array data type conversions within the Links between Regions. It will require that the Output type of the sending Region must have the SAME type as the receiving Region's Input. One of the future PR's will be to provide some portion of automatic data type conversion within the Link.
So for now I am going to disable some of the LinkTests so I can finish this PR, but we need to go back and fix is.

and followup #136 (comment)

The NTA_BasicType enum is in nupic/types/Types.h
This is where these should be declared.
nupic/types/BasicType.cpp has some basic functions that deal with those types...like getting a text name for them, given the type, return the NTA_BasicType id, return the size of an element in that type. etc. The framework uses these to deal with these types.
nupic/ntypes/Array (and its base class ArrayBase.cpp) is the container that holds these buffer types. It needs to know about these two types. Most of the Network framework knows about Array so that it does not need to know about each of the different buffer types. So by wrapping your new buffer types in an Array you can pass your sparse or SPR buffers through the framework to the various Regions.
The OPF framework mentioned in the API (all Python code) relies on this facility in the Network framework in order to do its job so we have to be careful that we don't break it.
If you want me to do those changes I can. But creating a new field in the Spec is not the way to do it. Lets use the architecture as it was intended.

[breznak]

So for now I am going to disable some of the LinkTests so I can finish this PR, but we need to go back and fix is.

is this disabled because of this issue, or the conversion?

Now...the current implementation does not allow array data type conversions within the Links between Regions. It will require that the Output type of the sending Region must have the SAME type as the receiving Region's Input. One of the future PR's will be to provide some portion of automatic data type conversion within the Link.

If we remove the sparse bool and implement a NTA_BasicType_Sparse:

So by wrapping your new buffer types in an Array you can pass your sparse or SPR buffers through the framework to the various Regions.

Isn't that too much extra work&overhead, just compared to the isSparse field?

And an argument for keeping the status quo (with sparse bool):

The OPF framework mentioned in the API (all Python code) relies on this facility in the Network framework in order to do its job so we have to be careful that we don't break it.
If you want me to do those changes I can. But creating a new field in the Spec is not the way to do it. Lets use the architecture as it was intended.

the isSparse does not come from our community-era changes, it's in Numenta master. Meaning the API doc is out-of-date and correcting/removing sparse would actually break the py code and any applications currently using it.

Hence, my vote to keep as is. Or at least keep now and might change if someone has objections. See if we can run #137

[dkeeney]

the isSparse does not come from our community-era changes, it's in

Numenta master. Oh, that complicates things. If we don't use NTA_BasicType to handle different data types or format of arrays, then we don't have a means to handle the SDR object either. There is no way for the framework to pass an SDR from one region to another.

…

On Mon, Dec 3, 2018 at 6:00 AM breznak ***@***.***> wrote: So for now I am going to disable some of the LinkTests so I can finish this PR, but we need to go back and fix is. is this disabled because of this issue, or the conversion? Now...the current implementation does not allow array data type conversions within the Links between Regions. It will require that the Output type of the sending Region must have the SAME type as the receiving Region's Input. One of the future PR's will be to provide some portion of automatic data type conversion within the Link. If we remove the sparse bool and implement a NTA_BasicType_Sparse: So by wrapping your new buffer types in an Array you can pass your sparse or SPR buffers through the framework to the various Regions. Isn't that too much extra work&overhead, just compared to the isSparse field? And an argument for keeping the status quo (with sparse bool): The OPF framework mentioned in the API (all Python code) relies on this facility in the Network framework in order to do its job so we have to be careful that we don't break it. If you want me to do those changes I can. But creating a new field in the Spec is not the way to do it. Lets use the architecture as it was intended. the isSparse does not come from our community-era changes, it's in Numenta master. Meaning the API doc is out-of-date and correcting/removing sparse would actually break the py code and any applications currently using it. Hence, my vote to keep as is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBa_6bPpTxxu6SQRZO4o4gpz5bbfz1tks5u1S55gaJpZM4Y-nyW> .

[breznak]

we can use NTA_BasicType for SDR class, but keep the "hack" with isSparse from numenta/master (I'm afraid it was me who introduced it back then)

Or we just do it right (tm)...and potentially break some things (let's keep that after #137 ) to see if the popular nupic .py repo is affected

[dkeeney]

Or we just do it right (tm)...and potentially break some things (let's keep that after #137 ) to see if the popular nupic .py repo is affected

I would really like to do this right if we don't break everyone. But definitely after #137.

[breznak]

@dkeeney I think this was resolved by some recent PR (which?) Can we close this?

[dkeeney]

Yes, it was resolved in #230.