Cereal for Array, ArrayBase #372

dkeeney · 2019-04-05T21:32:57Z

This adds Array and ArrayBase to Cereal serialization.

…nto cereal-arraybase

breznak

wow, this was a more complex serialization, hope ArrayBase is the only (bad) exception.
I don't understand ArrayBase that deeply, but:

I like the cleanup of veriables & methods
if the new cerialization works, we're golden 👌
please use c++ casts (static_cast<T>(val)) at the few places, rather than plain C-casts ( (T)val)

src/nupic/engine/Link.cpp

src/nupic/ntypes/Array.hpp

src/nupic/ntypes/ArrayBase.cpp

breznak · 2019-04-05T22:28:55Z

src/nupic/ntypes/ArrayBase.cpp

  NTA_CHECK(type_ == NTA_BasicType_SDR) << "Does not contain an SDR object";
  if (buffer_ == nullptr) {
    std::vector<UInt> zeroDim;
    zeroDim.push_back(0u);
    allocateBuffer(zeroDim);  // Create an empty SDR object.
  }
-  SDR& sdr = *((SDR *)buffer_.get());
+  sdr::SDR& sdr = *((sdr::SDR *)buffer_.get());


nit, can you use static_cast ? (or other relevant c++ casting), it does better checks that the plain C-cast

nit, can you use static_cast ?

I just ever got into the habit of using static_cast. I know what C casting does. Not so sure about the C++ casting. But I guess I should lean to use the new stuff.

have a look, it's a nice c++ feature: https://stackoverflow.com/questions/332030/when-should-static-cast-dynamic-cast-const-cast-and-reinterpret-cast-be-used

My layman's version:

it's possible to grep sourcecode for the cast

no performance penalty

does some checks

1st goto version: static_cast<T>(val)

..I'd like to see these implemented for the PR

src/nupic/ntypes/ArrayBase.cpp

breznak · 2019-04-05T22:32:34Z

src/nupic/ntypes/ArrayBase.cpp

-      a.setCount(maxsize);
-  }
+	// TODO:  Comment this out until we are sure that it is not needed.
+  //if (offset == 0) {


what was this for?Why we don't use it but you wanna keep that?

It was for handling Fan-in merge when buffers could be variable length (Sparse arrays). I don't think any other code was depending on this logic being here so if nothing breaks I will remove the commented out code.

src/nupic/utils/StringUtils.cpp

src/test/unit/ntypes/ArrayTest.cpp

dkeeney · 2019-04-06T00:03:17Z

I will need another review before the master changes again. Thanks.

breznak · 2019-04-06T00:35:51Z

I will need another review before the master changes again. Thanks.

I will, but you haven't made changes on the last review (?)

…nto cereal-arraybase

dkeeney · 2019-04-06T14:08:19Z

I will, but you haven't made changes on the last review (?)

So, you really are going to make me change my coding style.
Your are right you know....but that does not make it any easier.

dkeeney · 2019-04-06T14:11:31Z

Hmmm,
When I compile now after pulling in the master, I am getting the following:

2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(141): warning C4267: 'argument': conversion from 'size_t' to 'nupic::algorithms::connections::CellIdx', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(160): warning C4267: 'initializing': conversion from 'size_t' to 'nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(160): warning C4267: 'initializing': conversion from 'size_t' to 'const nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(286): warning C4267: 'argument': conversion from 'size_t' to 'nupic::Int', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(571): warning C4267: 'argument': conversion from 'size_t' to '_Ty', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(575): warning C4267: 'argument': conversion from 'size_t' to '_Ty', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(585): warning C4267: 'initializing': conversion from 'size_t' to 'nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(585): warning C4267: 'initializing': conversion from 'size_t' to 'const nupic::UInt32', possible loss of data

and lots of them

I am going to assume that is not me.

dkeeney · 2019-04-06T14:56:23Z

I am going to be away from my computer for the rest of the day.
Will have the casts fixed shortly.

breznak

thanks for the c++ casting fixes, David, the code looks 💯 now!!

I've fixed the un/signed compared in the test;
- gtest is really bad at hinting where (such small) problem happens, so these are always a PIA

src/test/unit/ntypes/ArrayTest.cpp

dkeeney · 2019-04-06T23:33:22Z

I have lots more cast fixes coming...so don't push to master yet.

breznak

Great having the harderst class Cerealed 👍 👍
Thank you!

breznak · 2019-04-06T23:35:39Z

I have lots more cast fixes coming...so don't push to master yet.

oh, you..stop it, you are spoiling me with goodness 😄

This PR has already 👍 from me, so do as you want. You can merge and make the other casts a separate PR.

dkeeney · 2019-04-06T23:41:45Z

I am still getting errors caused by the type of CellIdx

2>TemporalMemory.cpp
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(141): warning C4267: 'argument': conversion from 'size_t' to 'nupic::algorithms::connections::CellIdx', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(160): warning C4267: 'initializing': conversion from 'size_t' to 'nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(160): warning C4267: 'initializing': conversion from 'size_t' to 'const nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(286): warning C4267: 'argument': conversion from 'size_t' to 'nupic::Int', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(571): warning C4267: 'argument': conversion from 'size_t' to '_Ty', possible loss of data
2>        with
2>        [
2>            _Ty=std::seed_seq::result_type
2>        ]
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(575): warning C4267: 'argument': conversion from 'size_t' to '_Ty', possible loss of data
2>        with
2>        [
2>            _Ty=std::seed_seq::result_type
2>        ]
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(585): warning C4267: 'initializing': conversion from 'size_t' to 'nupic::UInt32', possible loss of data
2>c:\projects\ai\cppb\src\nupic\algorithms\temporalmemory.cpp(585): warning C4267: 'initializing': conversion from 'size_t' to 'const nupic::UInt32', possible loss of data

dkeeney · 2019-04-06T23:42:20Z

Does your compiler not show those warnings?

dkeeney · 2019-04-06T23:43:01Z

And....

7>TemporalMemoryTest.cpp
7>c:\projects\ai\cppb\src\test\unit\algorithms\temporalmemorytest.cpp(1360): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
7>c:\projects\ai\cppb\src\test\unit\algorithms\temporalmemorytest.cpp(1368): warning C4267: 'initializing': conversion from 'size_t' to 'nupic::Int', possible loss of data
7>c:\projects\ai\cppb\src\test\unit\algorithms\temporalmemorytest.cpp(1560): warning C4267: '/=': conversion from 'size_t' to '_Ty', possible loss of data
7>        with
7>        [
7>            _Ty=std::seed_seq::result_type
7>        ]

dkeeney · 2019-04-06T23:43:54Z

To be consistent....we should make ALL size related things size_t. This includes the dimensions.

dkeeney · 2019-04-06T23:45:59Z

Oh, and what about the sparse array elements... aren't they also size related? Probably should keep those at UInt32.

breznak · 2019-04-06T23:55:48Z

I am still getting errors caused by the type of CellIdx

No! (gcc 7.3, linux), I thought those had gone away for you. Try a clean build...

Oh, and what about the sparse array elements... aren't they also size related? Probably should keep those at UInt32.

we can't end up with everything size_t'ed :) SDR_sparse_t elements (are now sdr::ElemSparse type) and are/must be matched with connections::CellIdx (so yes, now UInt32, but can change)

dkeeney · 2019-04-07T00:00:20Z

I am still getting errors caused by the type of CellIdx

I see them only when I do a clean build.

breznak · 2019-04-07T00:03:11Z

I see them [CellIdx typedef related errors] only when I do a clean build.

That's very strange! can you open an issue for that? Event the CI guys here are satisfied 🤔

dkeeney · 2019-04-07T00:04:01Z

I am running a unit test and if that passes I will push some more changes.

dkeeney · 2019-04-07T00:05:36Z

crashed.
Guess I have more work to do.

breznak · 2019-04-07T00:07:51Z

crashed.
Guess I have more work to do.

let's merge this, and the other can come later..

dkeeney · 2019-04-07T00:16:35Z

Maybe you can help with this....

  template<class Archive>
  void load_ar(Archive & ar) {
    std::vector<UInt>& v = *(reinterpret_cast<std::vector<UInt>*>(this));
    ar(v);
  }

Trying to serialize the Dimensions class. This is a subclass of std::vector
What I thought I could do was use the this pointer to tell Cereal that this is really a vector.

The above code crashes deep inside the cereal templates (about 5 levels deep).
Access violation reading location 0xFFFFFFFFFFFFFF in other words it was lost.

So obviously it does not like something.

I have a social engagement in 15 min so I will take this up in the morning. Will push what I have so you can review all of casts..... you asked for them.

This reverts commit 7e2c3f1.

This reverts commit 7524aa4.

breznak · 2019-04-07T01:00:21Z

can review all of casts..... you asked for them.

I've reverted the last 2 changes in this PR, to merge this as "cereal", and I'll put them in a new one.

std::vector& v = (reinterpret_cast<std::vector>(this));

maybe a static/dynamic cast?

PS: the new extra casts break some tests, we can fix them, but no need to sweat it. I meant please use the casts for new code, but we don't have to convert all the existing codebase atonce now.

breznak

👍

dkeeney · 2019-04-07T14:14:46Z

I've reverted the last 2 changes in this PR, to merge this as "cereal", and I'll put them in a new one.

Ok, I will continue with the Cereal conversions. I have one more PR for Spec and friends (Collection, Scaler, and Value). That would complete all of the lower level modules in ntypes and types. Then I can start in on algorithms, followed by regions and then NetworkAPI.

Array, ArrayBase

7d28742

dkeeney changed the title ~~Array, ArrayBase~~ Cereal for Array, ArrayBase Apr 5, 2019

dkeeney self-assigned this Apr 5, 2019

dkeeney added the serializable label Apr 5, 2019

Merge branch 'master' of https://github.com/htm-community/nupic.cpp i…

719a27e

…nto cereal-arraybase

breznak requested changes Apr 5, 2019

View reviewed changes

Merge branch 'master' of https://github.com/htm-community/nupic.cpp i…

311557f

…nto cereal-arraybase

dkeeney and others added 4 commits April 6, 2019 14:41

replaced C casts with C++ casts in Array, ArrayBase

f0e2af8

Merge branch 'master_community' into cereal-arraybase

5076f7d

ArrayBase test: fix un/signed comparisons

26c13c6

ArrayBase: test turn off debug

c314b40

breznak reviewed Apr 6, 2019

View reviewed changes

src/test/unit/ntypes/ArrayTest.cpp Outdated Show resolved Hide resolved

breznak previously approved these changes Apr 6, 2019

View reviewed changes

More C casts to C++ casts.

7524aa4

More C casts changes.

7e2c3f1

dkeeney dismissed breznak’s stale review via 7e2c3f1 April 7, 2019 00:17

breznak mentioned this pull request Apr 7, 2019

STL compatible size(), dimensions() return size_t #357

Open

breznak added 2 commits April 7, 2019 02:28

Revert "More C casts changes."

1de2e99

This reverts commit 7e2c3f1.

Revert "More C casts to C++ casts."

01c6e09

This reverts commit 7524aa4.

breznak approved these changes Apr 7, 2019

View reviewed changes

breznak merged commit 8eacb12 into master Apr 7, 2019

breznak deleted the cereal-arraybase branch April 7, 2019 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cereal for Array, ArrayBase #372

Cereal for Array, ArrayBase #372

dkeeney commented Apr 5, 2019

breznak left a comment

breznak Apr 5, 2019

dkeeney Apr 5, 2019

breznak Apr 5, 2019

breznak Apr 6, 2019

breznak Apr 5, 2019

dkeeney Apr 5, 2019

dkeeney commented Apr 6, 2019

breznak commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

breznak left a comment

dkeeney commented Apr 6, 2019

breznak left a comment

breznak commented Apr 6, 2019 •

edited

Loading

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

breznak commented Apr 6, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

dkeeney commented Apr 7, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

breznak left a comment

dkeeney commented Apr 7, 2019

Cereal for Array, ArrayBase #372

Cereal for Array, ArrayBase #372

Conversation

dkeeney commented Apr 5, 2019

breznak left a comment

Choose a reason for hiding this comment

breznak Apr 5, 2019

Choose a reason for hiding this comment

dkeeney Apr 5, 2019

Choose a reason for hiding this comment

breznak Apr 5, 2019

Choose a reason for hiding this comment

breznak Apr 6, 2019

Choose a reason for hiding this comment

breznak Apr 5, 2019

Choose a reason for hiding this comment

dkeeney Apr 5, 2019

Choose a reason for hiding this comment

dkeeney commented Apr 6, 2019

breznak commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

breznak left a comment

Choose a reason for hiding this comment

dkeeney commented Apr 6, 2019

breznak left a comment

Choose a reason for hiding this comment

breznak commented Apr 6, 2019 • edited Loading

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

dkeeney commented Apr 6, 2019

breznak commented Apr 6, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

dkeeney commented Apr 7, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

dkeeney commented Apr 7, 2019

breznak commented Apr 7, 2019

breznak left a comment

Choose a reason for hiding this comment

dkeeney commented Apr 7, 2019

breznak commented Apr 6, 2019 •

edited

Loading