stan-dev
De-duplicating var<matrix_cl> and var<Eigen> functions
Description
I was going through adding some simple var matrix functions like
template <typename T, require_matrix_t<T>* = nullptr>
inline Eigen::Index cols(const T& m) {
return m.cols();
}
It just requires changing the previous require_eigen_t<T> to require_matrix_t<T>. But I'm looking at it like, why can't this support both var<Eigen::Matrix> and var<matrix_cl>?
Example
What I'm thinking is something like
template <typename T, require_matrix_t<Engine::All, T>* = nullptr>
inline Eigen::Index cols(const T& m) {
return m.cols();
}
Where the first argument is an enum that decides what backend the function supports.
enum class Engine {Eigen, OpenCL, All};
I think if we added a few methods to the matrix_cl class we could even use the same code for things like acos() below.
template <typename VarMat, require_var_matrix_t<Engine::All, VarMat>* = nullptr>
inline auto acos(const VarMat& x) {
return make_callback_var(
x.val().array().acos().matrix(), [x](const auto& vi) mutable {
x.adj().array()
-= vi.adj().array() / (1.0 - (x.val().array().square())).sqrt();
});
}
alt to adding the methods we could just change the above like functions to use stan prim functions.
@t4c1 @rok-cesnovar how do yinz feel about this? I think with something like this we could de-dup a lot of work
Current Version:
v4.1.0
If this could be done without a lot of complications and it would open up the possibility to deduplicate a lot of code then I am for it. If its for 5-10 functions then I am not sure its worth it.
I am a bit afraid something will pop up later that will require reduplicating everything. I would definitely give this a low priority.
What I'm thinking is something like
That part looks more or less ok. I am just not sure if we want to do type conversions like that - Eigen uses 64 bit ints for sizes, while matrix_cls use 32-bit ints.
Where the first argument is an enum that decides what backend the function supports.
To me that looks like unnecessary abstraction that will just complicate code. You can just as well use requires without that.
I think if we added a few methods to the matrix_cl class we could even use the same code for things like acos() below.
I would vote against that. To do it for all functions it would require having matching interfaces for matrix_cl and Eigen::Matrix. That would be a lot of changes and therefore a lot of opportunities for something to go wrong. Either bugs or performance regressions. Overall this seems a lot of work for only a little benefit. Also I agree with Rok's concern - we might need to reduplicate things later.
That part looks more or less ok. I am just not sure if we want to do type conversions like that - Eigen uses 64 bit ints for sizes, while matrix_cls use 32-bit ints.
For that particular example we could just have it return auto.
Where the first argument is an enum that decides what backend the function supports.
To me that looks like unnecessary abstraction that will just complicate code. You can just as well use requires without that.
I'm 50/50 on the idea. To me it just seems like there could be a good number of functions that can work for both. I'm also finding that some of our template metaprogramming is getting wordy
I think if we added a few methods to the matrix_cl class we could even use the same code for things like acos() below.
I would vote against that. To do it for all functions it would require having matching interfaces for matrix_cl and Eigen::Matrix. That would be a lot of changes and therefore a lot of opportunities for something to go wrong. Either bugs or performance regressions. Overall this seems a lot of work for only a little benefit. Also I agree with Rok's concern - we might need to reduplicate things later.
Yeah I'm fine with not doing all those methods, we could just call the Stan prim functions