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

add support of dynamically sized Eigen types #1541

Merged
merged 11 commits into from
Jan 3, 2025

Conversation

WanZhiQiu-ac
Copy link
Contributor

I use b[ix++] = '['; because I saw something like

if (const auto k = ix + write_padding_bytes; k > b.size()) [[unlikely]] {
   b.resize(2 * k);
}

so this might be safe. Not so sure.

A quick test:

#include <Eigen/Core>
#include <iostream>

#include "glaze/ext/eigen.hpp"
#include "glaze/glaze.hpp"

class Example
{
   int a{1}, b{2}, c{3}, d{4}, e{5}, f{6}; // ... and many many more

   friend struct glz::meta<Example>;

  public:
   Eigen::MatrixXd mat;
   Eigen::VectorXd vec;
};

template <>
struct glz::meta<Example>
{
   using T = Example;
   static constexpr auto value = object(&T::a, &T::b, &T::c, &T::d, &T::e, &T::f, &T::mat, &T::vec);
};

int main()
{
   Example obj1{}, obj2{};
   obj1.mat.resize(2, 3);
   for (int j = 0; j < 2 * 3; ++j) {
      obj1.mat(j) = j + 1.01;
   }
   obj1.vec.resize(3);
   obj1.vec << 1, 2, 3.33333;

   std::string buffer = glz::write_json(obj1).value();
   std::cout << buffer << '\n';

   assert(obj2.mat.size() == 0);
   assert(obj2.vec.size() == 0);

   auto ec = glz::read_json(obj2, buffer); // populates s from buffer
   if (ec) {
      std::cout << "error" << std::endl;
   }
   assert(obj2.mat.rows() == 2);
   assert(obj2.mat.cols() == 3);
   assert(obj2.vec.size() == 3);
   assert(obj1.mat == obj2.mat);
   assert(obj1.vec == obj2.vec);
}

@stephenberry
Copy link
Owner

Thanks for this! I can tweak the code a bit to handle buffer bounds safety. Could you add some unit tests to the tests/eigen_test/eigen_test.cpp in this pull request?

The simple solution for writing safely is dump<'['>(b, ix);

@stephenberry
Copy link
Owner

@WanZhiQiu-ac, I guess I just collided with you, but I merged our changes.

@stephenberry
Copy link
Owner

@WanZhiQiu-ac, I'll add some more unit tests. I limited the specialization for matrices with both rows and columns being dynamic, because Glaze already supported dynamic vectors in a way that didn't need to serialize the extents.

@stephenberry
Copy link
Owner

@WanZhiQiu-ac, I've added more tests, but some are breaking, so I need to work through them.

@stephenberry
Copy link
Owner

@WanZhiQiu-ac
So, everything seems to be working now. I will note that this approach does not encode the layout (row or column major). However, transposing the data is quite expensive and it is better to serialize/deserialize in the same format. But, if you think we should encode the layout (and maybe just error if the layout doesn't match) then we can update the code. The BEVE code handles the layout, just not the JSON.

I'm going to merge this in, but make another pull request if you want to handle layout checking or transposing. But, this should now serialize/deserialize dynamic matrices.

@stephenberry stephenberry merged commit 0ba9d36 into stephenberry:main Jan 3, 2025
12 checks passed
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