Add trace_dot in linalg #4176
Add trace_dot in linalg #4176
Conversation
| obj[iter] += linalg::trace( | ||
| linalg::element_prod( | ||
| linalg::matrix_prod(L, L, true, false), gradient)); | ||
| obj[iter] += |
There was a problem hiding this comment.
Is this the same as before? Doesnt seem to me but maybe I am blind :)
There was a problem hiding this comment.
They are different. Actually I was wrong in #4167. Now it is the same as the original one.
shogun/src/shogun/metric/LMNN.cpp
Line 113 in 431e56b
There was a problem hiding this comment.
EHM!!!
Why did the tests pass? Or are there none?
There was a problem hiding this comment.
Well, here the trace is used as the termination condition. Seems that it doesn't stop until reaching maximum number of iterations when trace is wrong in tests.
There was a problem hiding this comment.
I see now! makes sense
Ok, this definitely should be tested somehow (at least in a unit test)
Wanna do send another PR for that?
There was a problem hiding this comment.
Check the history of that line, it changed back and forth IIRC
There was a problem hiding this comment.
I checked and only found the three mentioned here: the original with TRACE plus the two in this diff.
There was a problem hiding this comment.
The original with TRACE is correct, the former one in this diff is wrong.
There was a problem hiding this comment.
Where is the difference between them, @vinx13?
There was a problem hiding this comment.
We are calculating trace of L' * L * gradient, which is sum( (L'L) . gradient' ), instread of trace( (L'L) . gradient) that appears in the previous commit
| template <typename T> | ||
| T trace_dot(const SGMatrix<T>& A, const SGMatrix<T>& B) | ||
| { | ||
| return sum(element_prod(A, transpose_matrix(B))); |
There was a problem hiding this comment.
is transpose matrix changing the memory (it shouldnt)
Also, the API seems inconsistent, for some methods we pass flags for whether matrices should be transposed. And for others, this flag doesnt exist and we have to transpose matrices from the outside ?
There was a problem hiding this comment.
transpose matrix creates a new matrix.
The API is indeed inconsistent, maybe we should add transpose flags to element_prod ?
|
What is the status of this one then? :) |
|
This one would be ready for review after cleaning up calls to transpose_matrix. |
|
ok ping me here once that is the case |
vinx13
force-pushed
the
feature/linalg_trace_dot
branch
from
f4183d5
to
04d7e73
Compare
|
@karlnapf Ready for review now :) |
| @@ -1734,6 +1734,23 @@ TEST(LinalgBackendEigen, SGMatrix_trace) | |||
| EXPECT_NEAR(trace(A), tr, 1e-15); | |||
| } | |||
|
|
|||
| TEST(LinalgBackendEigen, SGMatrix_tract_dot) | |||
|
typo fix and travis green and we can merge this :) |
|
restarted the hickup builds...should be fine soon |
* Implemented trace_dot in linalg * Use trace_dot in LMNN
trace_dotin LMNN