Skip to content

Cleaning up linear access functions #2245

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

Open
bbbales2 opened this issue Dec 7, 2020 · 4 comments
Open

Cleaning up linear access functions #2245

bbbales2 opened this issue Dec 7, 2020 · 4 comments

Comments

@bbbales2
Copy link
Member

bbbales2 commented Dec 7, 2020

Now that we're exposing expressions in the language, we need to clean up functions that depend on linear access in Eigen types.

Specifically, with a simple Eigen type like Eigen::MatrixXd, it is possible to instead of looping over ij like so:

Eigen::MatrixXd A;
for(size_t i = 0; i < A.size(); ++i) {
  for(size_t i = 0; j < A.size(); ++j) {
    ... A(i, j) ...;
  }
}

to just do:

Eigen::MatrixXd A;
for(size_t i = 0; i < A.size(); ++i) {
  ... A(i) ...;
}

This was convenient because the same thing worked with matrices, vectors, row vectors.

Now that we have expressions and maps and such we need to be more careful.

There have been two pulls addressing issues like this so far, #2205 and #2223

I made this issue so we would vaguely have a place to discuss:

  1. How we should be testing for this so it doesn't bite us
  2. Track known issues that exist (until there's some sort of general solution in place)
  3. How do we handle row major vs. column major -- now our double loops will be explicitly bad in some cases

And and I do not want to stall #2223 unnecessarily to try to fix everything in one go.

Current Version:

v3.4.0

@t4c1
Copy link
Contributor

t4c1 commented Dec 7, 2020

How we should be testing for this so it doesn't bite us

I already added testing to expression testing framework. Although varmat functions are not tested using it (yet?).

How do we handle row major vs. column major -- now our double loops will be explicitly bad in some cases

The simple option is to replace loops with expressions. The faster option is to have both row-major and col-major loops and pick depending if more expressions are row or col major.

@bbbales2
Copy link
Member Author

bbbales2 commented Dec 7, 2020

I already added testing to expression testing framework. Although varmat functions are not tested using it (yet?).

Oh is that testing 2x2 matrices and the gradients and stuff? I remember merging the block tests, but I assumed that was more a compile thing.

The simple option is to replace loops with expressions.

So this should be our go to? And then we add require types so we can detect the row/col majorness?

@t4c1
Copy link
Contributor

t4c1 commented Dec 7, 2020

Oh is that testing 2x2 matrices and the gradients and stuff? I remember merging the block tests, but I assumed that was more a compile thing.

It is actually a simple test on 1x1 matrices, but if it compiles it should work.

So this should be our go to?

I will try to prepare a general framework that will pick row/col loops depending on expressions. Although it is not that high on my todo. Maybe in a week or so.

@t4c1 t4c1 closed this as completed Dec 7, 2020
@t4c1 t4c1 reopened this Dec 7, 2020
@bbbales2
Copy link
Member Author

bbbales2 commented Dec 7, 2020

The reason I asked about the 2x2 is that I didn't find the problem with the x + x.transpose() thing until I was working on a larger than 1x1 problem.

I could imagine in the test framework we take every function that takes a matrix of size NxN as input, build an (N + 1)x(N + 1) matrix that holds it, and then do the test on the NxN sub-block of that matrix.

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

No branches or pull requests

2 participants