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

Use PermutationMatrix instead of indices #475

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

akleeman
Copy link
Collaborator

@akleeman akleeman commented Feb 2, 2024

The sparse Gaussian process implementations used to store a vector of indices which represented a permutation matrix. The reason it did this was that the PermutationMatrix isn't directly serializable, so you end up needing to copy the indices when you deserialize one of these models. Storing the matrices this way led to a number of places with homegrown permutation matrix application, you'd see things like:

  for (Eigen::Index i = 0; i < permutation_indices.size(); ++i) {
    lhs.row(i) = rhs.row(permutation_indices.coeff(i));
  }

These loops are not easy to interpret. In this case it's computing lhs = rhs * P.transpose(), but the transpose is not evident from the code. It's also really easy to introduce a bug in these situations (if the rhs isn't the same shape as how lhs was allocated you can encounter some really tricky memory handling errors).

This PR adds a serialization method for a raw PermutationMatrix<> and changes that to be the type stored in sparse GP fits. While perhaps slightly less efficient in the few situations where we now need to copy indices, that should only really matter when the size of the indices is huge, in which case these copy operations will probably be dwarfed by large linear algebra operations.

@akleeman akleeman requested a review from jbangelo February 2, 2024 19:46
@akleeman akleeman force-pushed the akleeman/permutation_matrix branch from 9b0bb1a to 0c04ad0 Compare February 2, 2024 19:55
@akleeman akleeman force-pushed the akleeman/permutation_matrix branch from 0c04ad0 to cb0c7e9 Compare February 2, 2024 20:44
Copy link
Contributor

@peddie peddie left a comment

Choose a reason for hiding this comment

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

This is a great cleanup, thanks.

@akleeman akleeman merged commit 2b99854 into master Feb 5, 2024
9 checks passed
@akleeman akleeman deleted the akleeman/permutation_matrix branch February 5, 2024 18:46
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.

3 participants