Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cereal for TestNode #419

Merged
merged 8 commits into from
Apr 20, 2019
Merged

Cereal for TestNode #419

merged 8 commits into from
Apr 20, 2019

Conversation

dkeeney
Copy link

@dkeeney dkeeney commented Apr 17, 2019

This adds Cereal to the TestNode Region Implementation as the first plugin class.
However, a plugin cannot be tested without its corresponding Region class which wraps it so this includes the Region class as well. Instantiating a plugin using Cereal deserialization required some changes to the plugin interface, and corresponding changes to all plugins. As a consequence there are a lot of files changed but must of it are stubs and parallel routines for passing the Archive rather than a stream object.

Trying to keep from breaking the original save/load serialization was a challenge.

@dkeeney dkeeney self-assigned this Apr 18, 2019
@dkeeney
Copy link
Author

dkeeney commented Apr 18, 2019

Finally ready for review when one of you get a chance.
Since Cereal serialization had to be in the .hpp, the forward declarations were not sufficient and I had to move include files around and the order of include became important. I found a combination that worked.

Copy link
Collaborator

@ctrl-z-9000-times ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@dkeeney dkeeney merged commit 0fcf1ba into master Apr 20, 2019
@breznak breznak deleted the serial-TestNode branch April 23, 2019 08:12
Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Thank you @dkeeney , this must have been a lot of interconnected work!

  • please see the comments about commented-out tests

@@ -114,7 +114,7 @@ class Dimensions : public std::vector<UInt>, public Serializable {
if (humanReadable && isInvalid()) ss << "(Invalid) ";
return ss.str();
}

/****
CerealAdapter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rm if unused?

@@ -154,29 +154,21 @@ TEST_F(DimensionsTest, Overloads) {

}

// Dimensions object is treated as a vector<UInt32> by Cereal
// so it is serializable without the save/load functions.
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to enable the test?

@@ -469,6 +502,12 @@ class Region : public Serializable {
// common method used by both constructors
// Can be called after nodespec_ has been set.
void createInputsAndOutputs_();
void saveDims(std::map<std::string,Dimensions>& outDims,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, should have used saveDims_, but that's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants