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

Serializable LDLT Decomposition. #21

Merged
merged 6 commits into from
Jun 8, 2018

Conversation

akleeman
Copy link
Collaborator

@akleeman akleeman commented Jun 7, 2018

Adds an extention of Eigen's LDLT which is serializable and only serializes the lower triangular portion.

Changes include:

  • Generalization of the existing Matrix serialization routines to allow different scalar types. What were previously separate serializations routines for Matrix3d and MatrixXd are now handled in a single case which also allows for different internal types.
  • Addition of the SerializableLDLT class which wraps LDLT<MatrixXd>.
  • Use of SerilizableLDLT in GaussianProcessFit.
  • Extended test_serilalize to test more than just the models.

@akleeman akleeman force-pushed the improve_gp_serialization branch 3 times, most recently from d4c7dee to fe33d34 Compare June 7, 2018 17:05
@akleeman akleeman force-pushed the improve_gp_serialization branch 5 times, most recently from 57e18e0 to e3aa94f Compare June 7, 2018 23:57
@akleeman akleeman force-pushed the improve_gp_serialization branch from e3aa94f to ec0471b Compare June 8, 2018 00:03
Copy link
Contributor

@benjamin0 benjamin0 left a comment

Choose a reason for hiding this comment

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

Looks great!

/*
* The subsequent save/load methods catch the serialization methods
* for arbitrary Eigen::Matrix* types. The general idea is that each
* row is saved as a Eigen::Vector.
Copy link
Contributor

Choose a reason for hiding this comment

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

How was it done before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was the same idea, but with explict use of VectorXd. Now it's via template specialization with _Cols=1.

size_type rows;
archive(cereal::make_size_tag(rows));
/*
* In order to determine the size of a matrix, we have to first determine
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier to serialize the dimensions instead of needing to scan through?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we could. There's a bit of a trick going on with cereal here. By adding a SizeTag (via make_size_tag) cereal will just serialize a list and upon loading it'll determine the size tag from the length of the data. Without this it'll serialize as a map ({'value0': 0, 'value1': 1, ...}) which is clearly not an efficient storage technique. What I could do is store redundant information. Ie, store a SizeTag and a shape. Think that's better?

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, I'm fine with whichever you think is easiest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alright, I changed it around so the rows and cols are stored explicitly. Shouldn't add much overhead in storage and makes the code easier to read (I think).

storage_size -= 2;
// We assume the matrix is square and compute the number of rows from the
// storage size.
double drows = (std::sqrt(static_cast<double>(storage_size * 8 + 1)) - 1) / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name these numbers so it's clearer what's going on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


SerializableLDLT(const LDLT<MatrixXd, Lower> &ldlt)
// Can we get around copying here?
: LDLT<MatrixXd, Lower>(ldlt){};
Copy link
Contributor

Choose a reason for hiding this comment

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

If this stores a unique pointer we could move it in which would save the copy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I understand correct, the problem with that is that it would require changing the way Eigen::LDLT works. As it stands all the storage is done in the base class. We might be able to do something like instantiate an empty Eigen::LDLT then point the internal members to pointers that are passed in ... but that feels wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah probably not worth it.

for (Eigen::Index i = 0; i < n; i++) {
const Eigen::VectorXd col_i =
inverse_cholesky.col(i).cwiseProduct(sqrt_diag);
;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra semicolon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch

this->matrixL().solveInPlace(inverse_cholesky);

Eigen::VectorXd sqrt_diag = this->vectorD();
for (Eigen::Index i = 0; i < n; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You could maybe use the Eigen::Array functions here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah interesting. Despite the appearance it seems like x.array().sqrt().matrix() doesn't add any runtime overhead ?!?

 * Store rows/cols explicitly for Matrix serialization.
 * Clarifications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants