Isolate Cereal #153

akleeman · 2019-08-28T19:58:50Z

It should be helpful to review each commit in order.

This change was motivated by discovering that 1) cereal headers are pretty heavy 2) using member serialization methods required including cereal in just about any albatross include. 3) the use of polymorphic registration generates a TON of symbols (alluded to in the "Registering in a header file" section here: https://uscilab.github.io/cereal/polymorphism.html) which slows down linking.

The change is effectively two parts, in the first we remove polymorphism from priors.hpp which was the only part of albatross that was actually using polymorphic pointers. This eliminates the need for making cereal work with polymorphism via the CEREAL_REGISTER_TYPE which blew up linking (it added ~10k symbols to our static libraries!!). Second all serialization methods were moved from member methods to free functions in a separate directory. The result is the ability to include albatross without including cereal, then only include albatross/Serialization in .cc files which require serialization.

NOTE: The result cut the unit test time in half!

…riant.

akleeman · 2019-08-29T08:44:51Z

I ended up using a strange pattern which seems to work quite nicely for this application. @jbangelo I'm curious if you have thoughts. Basically instead of using polymorphic pointers I switched to using variants which I then wrapped in a container. Ie, given a situation with

struct Base {
  int bar() const = 0;
}

struct X : Base {
  int bar() const override {return 1;};
}

instead of passing everything around as a std::shared_ptr<Base> I instead created a container:

struct PolymorphicContainer : Prior {
  int bar() const override {
    return foo.match([](const auto &f) {return f.bar()};
  }  
  variant<X, Y, Z> foo;
}

which also inherits from Base but implements all virtual methods using variant<>::match. The inheritance pattern seems compatible with the container and results in much more intuitive compiler errors if one of the types in the variant doesn't support a method it should.

The upside for this application is that it means I don't need to deal with registering polymorphic types and the mess that results from serializing pointers in cereal, this now happens with the serialization of the variant. The downside of this is that you need to know all the available types when the container is declared, so someone using albatross would not be able to add custom priors (though perhaps there's a work around for that).

make sense?

include all of albatross if you only want serialization of one part. For example, #include <albatross/Core> would only need: #include <albatross/serialize/Core> without this including albatross/Serialization would require including everything (since some of the types would not have been defined for the `serialize()` methods.

seth-swiftnav

Things look a lot cleaner around the priors now that they use variants. I can't talk to the header refactors. If it passes all unit tests and is faster, I'm happy.

As for not being able to create custom Priors, I'm neither concerned for the loss of custom priors nor do I think they can't be introduced.

seth-swiftnav · 2019-08-30T17:53:43Z

include/albatross/src/cereal/parameters.hpp

+  archive(cereal::make_nvp("prior", param.prior));
+};
+
+// template <class Archive>


Remove if unused?

seth-swiftnav · 2019-08-30T17:54:32Z

include/albatross/src/cereal/ransac.hpp

+  archive(cereal::make_nvp("indexing_function", strategy.indexing_function_));
+}
+
+// template <typename Archive, typename ModelType, typename StrategyType>


Remove if unused?

seth-swiftnav · 2019-08-30T17:57:26Z

tests/test_serialize.cc

                         FullJointDistribution, MeanOnlyJointDistribution,
                         FullMarginalDistribution, MeanOnlyMarginalDistribution,
-                         ParameterStoreType, Dataset, DatasetWithMetadata,
+                         Dataset, DatasetWithMetadata, ParameterStoreType,


I'm curious, why was this reorder required? It seems like neither Datasets or Parameter Stores depend on the other.

Ah, that was just left over from when I was commenting out and rearranging the order to debug some compile errors. I'll revert it to avoid confusion.

jbangelo

This seems like a reasonable approach. The only downside that I see is that your variant object will end up being large enough to store the largest value it could possibly contain. I can't easily tell from the PR, but if one or more of the possible values is significantly large it could be expensive copying it around, or at the very least would increase memory use. This probably isn't a problem though.

jbangelo · 2019-08-30T15:44:53Z

include/albatross/src/core/priors.hpp

+    variant<UninformativePrior, FixedPrior, NonNegativePrior, PositivePrior,
+            UniformPrior, LogScaleUniformPrior, GaussianPrior, LogNormalPrior>;
+
+struct PriorContainer : Prior {


Nit: this looks more like a class with all that functionality.

jbangelo · 2019-08-30T15:50:54Z

include/albatross/Serialization

+
+
+
+#endif


Nit, missing final new line.

jbangelo · 2019-08-30T15:51:30Z

include/albatross/Serialization

+#include "src/cereal/representations.hpp"
+#include "src/cereal/variant.hpp"
+
+//CEREAL_SPECIALIZE_FOR_ALL_ARCHIVES(albatross::ParameterStore, cereal::specialization::non_member_load_save );


Nit: Is commented line still needed?

Nope! thanks.

jbangelo · 2019-08-30T18:46:52Z

tests/test_indexing.cc

  to = DiagonalMatrixXd(x);
  set_subset(from, idx, &to);
-  EXPECT_EQ(to, expected);
+  EXPECT_TRUE(to == expected);


Why was this needed? Doesn't EXPECT_EQ use ==?

I have no clue why EXPECT_EQ doesn't work with Eigen::DiagonalMatrix<double, Dynamic>. It probably has something to do with the lack of == operator (I had to define a free standing one).

jbangelo · 2019-08-30T18:49:58Z

include/albatross/serialize/Common

+#include <cereal/types/string.hpp>
+#include <cereal/types/vector.hpp>
+
+#include "../src/cereal/traits.hpp"


Relative paths like this in include paths aren't great (they tend to break when porting to different systems(, and cause generally be avoided by setting the include paths you want. Would a path like albatross/src/cereal/traits.hpp work?

akleeman force-pushed the isolate_cereal branch 4 times, most recently from daec448 to 63f5273 Compare August 29, 2019 07:46

akleeman added 3 commits August 29, 2019 01:40

Switch Prior from being a pointer to an abstract class, to being a va…

6faa43f

…riant.

Move all serialization methods into the cereal directory

7790694

Remove unused files

b11b7e5

akleeman force-pushed the isolate_cereal branch from 63f5273 to b11b7e5 Compare August 29, 2019 08:44

akleeman requested review from jbangelo and seth-swiftnav August 29, 2019 08:44

akleeman added 2 commits August 29, 2019 02:20

Add similar includes for the utils which are commonly used on their own

fc8b4be

akleeman force-pushed the isolate_cereal branch from feec0f3 to fc8b4be Compare August 29, 2019 13:38

seth-swiftnav approved these changes Aug 30, 2019

View reviewed changes

Seth's changes

f62bb7e

jbangelo approved these changes Aug 30, 2019

View reviewed changes

akleeman added 2 commits August 30, 2019 12:49

Joseph's comments

1fd2bc3

Change relative paths to include paths

6b94299

akleeman merged commit 0e0c62f into swift-nav:master Aug 30, 2019

akleeman deleted the isolate_cereal branch August 30, 2019 20:07




		#endif No newline at end of file

Isolate Cereal #153

Isolate Cereal #153

Uh oh!

Conversation

akleeman commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akleeman commented Aug 29, 2019

Uh oh!

seth-swiftnav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akleeman commented Aug 28, 2019 •

edited

Loading