[GenABEL-dev] [Genabel-commits] r1671 - in branches/ProbABEL-0.50: examples src

[Diego Fabregat]

Hi guys,

If I may...
>>> +    /*
>>> +     in ProbABEL <0.50 this calculation was performed like t(X)*W
>>> +     This changed to W*X since this is better vectorized since the 
>>> left hand
>>> +     side has more rows: this introduces an additional transpose, 
>>> but can be
>>> +     neglected compared to the speedup this brings(about a factor 2 
>>> for the
>>> +     palinear with 1 predictor)
>>> +     */
>>> +    MatrixXd tXW = W_masked.masked_data->data * X.data;
>> I think the variable naming should be more apropriate here: tXW sounds
>> like X^t * W, but you store W * X in that variable.
> Yepp, your right it should be called tWX. We skip the transpose of W 
> since it is a symmetric matrix: however in terms of mathematics it 
> makes sense to call what we achieve. You can read in the code what we 
> do.  This might need some explanation in form of comments.
>>
>>> +    MatrixXd xWx = tXW.transpose() * X.data;
>> Similarly here, I'm not sure how to interpret xWx. Since you calculate
>> (W*X)^t * X a name like WXtX seems more reasonable.
>
> So this will be something like ttWXX ??? Any other good solution?
I don't know the context of the discussion, but what do you think about 
documenting the algorithm somewhere in the code (like at the top of the 
source file), giving simple names to the variables, and then just using 
those names instead of getting to a point where you have to juggle with 
cryptic variable names. For instance, in case you want to solve a 
least-squares problem  inv(X^T X) X^T y:

/*
  *  Algorithm for LSQ  [ b := inv(X^T X) X^T y ]
  *
  *  S := X^T X
  *  v := X^T y
  *  b := inv(S) y   (notice that this should be solved as a linear 
system, not explicitly inverting S)
  */

And then you can simply use S, v, and b in the code.

Best,
Diego