Cereal for TMRegion

[dkeeney]

Adds Cereal for TMRegion

Replaces #421

[dkeeney]

This is a new clean branch. The other one was so messed up I could not trust it.
This should be a simple PR. But nothing seems to be simple.

The Network class has two STL objects being serialized. One for regions and one for links. Whichever one is first is ok but the second one seems to be out of synch with the stream and crashes. I tried putting just a simple int field deserialized after the first STL object and it came up with the wrong value.
This is similar to (and may be the same) problem I was having when I tried to do my own iterations but I thought Cereal was doing them correctly. Apparently not.

This did not show up with the SPRegion PR because I did not happen to have any Links in the test. Well, I am out of time so I will have to finish this when I get back.

[breznak]

just a question to the TM. reset, otherwise looks good

[breznak]

the TM::reset() is questionable here, since it didnt have to be called prior to saving

[dkeeney]

The reset just assigns the raw TM pointer to the shared_ptr. Perhaps I should use a make_shared( ) here in this case since this is a simple allocation.

[breznak]

Perhaps I should use a make_shared( ) here in this case since this is a simple allocation.

ok, the name confused me into a TM.reset. Maybe rename the method to resetTMPointer or something? but it can stay as is

[breznak]

CI simple problem:

/Users/distiller/numenta/nupic.core/src/nupic/engine/Network.hpp:151:9: error: unused variable 'marker' [-Werror,-Wunused-variable]
int marker;

[dkeeney]

I pushed again without the marker field :) That was the field I serialized after the first STL. I missed the declaration when I took it back out.

[breznak]

This looks good, thank you David!

[dkeeney]

Funny how problems look so much simpler after coming back from a vacation.

• We can do sequences in Cereal. You have to be sure to use the correct size in make_size_tag( ) for the variable holding the iteration count. It must be cereal::size_type, not size_t. Also, for each iteration of a sequence you can output only one object. ar(a, b, c) is three objects and it will mess up the sequence count. So if you have more than one object for each sequence iteration, put them in a container and add save_ar( ) and load_ar( ) to the container. For an example, see TemporalMemory.hpp line 455.
• The regions_ member variable in the Network class is now a std::map( ) so it is easier to serialize. But this map is only seen inside the class. Outside the Network class regions is a Collection, obtained via the getRegions( ) call so this does not change the API.

[breznak]

Wow, this looks real good now! Thank you 👍

Funny how problems look so much simpler after coming back from a vacation.

indeed, sometimes we just need to take a rest from the problems.

So if you have more than one object for each sequence iteration, put them in a container and add save_ar( ) and load_ar( ) to the container. For an example, see TemporalMemory.hpp line 455.

this is great finding!

The regions_ member variable in the Network class is now a std::map( ) so it is easier to serialize. But this map is only seen inside the class. Outside the Network class regions is a Collection, obtained via the getRegions( ) call so this does not change the API.

Cool! The better API would use std::map instead, and internally Collection could have extended from std::map. Good as is now, do you want to break the API for the sake of getting rid of Collection, or leave things stable as is now?

[breznak]

this is perferct! Thanks 👍

[dkeeney]

The better API would use std::map instead, and internally Collection could have extended from std::map.

Collection contains both a vector of pairs and a map of indexes into the vector. It would be difficult to extend from map and get a class that can also be indexed like a vector to get a pair.

... for the sake of getting rid of Collection, ...

The Collection is still being used in several other places as well so it will take a while before we can get rid of it. Lets let it live in the API for now.

[dkeeney]

@breznak approved these changes

Thanks for doing the review for me.