refactored linalg's dot product #2367
Conversation
| @@ -121,123 +109,38 @@ struct dot<int,Backend::EIGEN3,Eigen::Matrix,T,Info...> | |||
| * @return the dot product of \f$\mathbf{a}\f$ and \f$\mathbf{b}\f$, computed | |||
| * as \f$\sum_i a_i b_i\f$ | |||
| */ | |||
| static T compute(vector_type a, vector_type b) | |||
| template <class Derived1, class Derived2> | |||
| static T compute(const Eigen::MatrixBase<Derived1>& a, const Eigen::MatrixBase<Derived2>& b) | |||
There was a problem hiding this comment.
@khalednasr why should we use two different Derived here? will this enable us to call dot on vectors of two different scalar types or this is to be flexible about one fixed and one dynamic sized vectors? Via redux wrappers I think only a single vector type is handled. If it helps in terms of flexibility, could you please add a tiny unit-test for that?
There was a problem hiding this comment.
@lambday it was for flexibility, but I forgot to add another vector type to the redux wrapper. I'll take care of it in the next patch.
There was a problem hiding this comment.
The return type is a headache though!
There was a problem hiding this comment.
maybe we can just always return float64_t?
There was a problem hiding this comment.
ummm nah that doesn't work for, say, complex128_t and float64_t :(
There was a problem hiding this comment.
ahh I see. I guess we should restrict it to one vector type for now.
There was a problem hiding this comment.
yep that's what I was thinking! but it doesn't harm to keep it like this though. So no need to change it.
There was a problem hiding this comment.
one can always directly call the linalg::implementation::dot if really necessary. But man will he be sorry for that :D
|
@khalednasr this looks superb and simpler. Travis had a upset stomach again so I restarted the build. I think this is the way to go for linalg. Please refactor the rest of the lib. Also, in the linalg::square method we compute element-wise square. So maybe change it to a better name? (sorry about that!). |
|
@khalednasr oh and please add your name in the files you modified :) |
refactored linalg's dot product
|
awesome! |
|
we should very soon merge this backend thing. then this year's gsoc project can refactor a bit against that. thoughts? |
|
@karlnapf absolutely. I guess just @khalednasr's next patch and then we're good to go! Later we'll iteratively add other modules. I am a bit concerned about merge conflicts - IIRC then OpenCV cmake stuffs and OpenCL/ViennaCL cmake stuff might be the culprit. Do we need to rebase against develop before merging with it? |
|
@lambday about the square() method, beside renaming, maybe it should be in another module(Elementwise.h for instance)? since it doesn't do any reduction. Also, I think that having it return a new matrix can be inefficient, and would be problematic since all the template specializations would have to return the same type of matrix, which would be bad for the gpu specializations. Instead I suggest having it be something like |
|
@khalednasr I agree. It totally makes sense to move that to another module. Maybe a more general name - say, Util or something. We could have matrix-matrix multiplication, matrix square, matrix logarithm etc there as well. Regarding second idea, I am not much aware of that. But if it indeed makes it efficient to have it this way then maybe its better. So +1 from me :) |
|
@lambday we only need to rebase if we cannot merge this. I expect that, but it shouldnt be a too big hassle |
<class Info,enum Backend,template<class,Info...>class Vector,class T,Info... I>to<enum Backend, class Vector>. This is simpler and works with any matrix type@lambday, @karlnapf, @lisitsyn, please take a look. If you guys are okay with those changes, I'll change the rest of the library in the same manner.