Code appears to not match paper

The following line of code appears to me to not match eq. 6 in your paper, http://stanford.edu/~hallac/TICC.pdf, :

Line 32 in 5788a14

    
           X_var = ( 1/(2*float(eta)) )*q*( numpy.diag(d + numpy.sqrt(numpy.square(d) + (4*eta)*numpy.ones(d.shape))) )*q.T

In your paper, within the square root is D^2 + 4 * rho * I, whereas in the code is numpy.square(d) + (4*eta)*numpy.ones(d.shape)), and since eta is 1/rho,

TICC/src/admm_solver.py

Line 39 in 5788a14

eta = 1/self.rho

, these appear to be different.

I checked the paper which your paper cites (https://arxiv.org/pdf/1111.0324.pdf), and eq. 3.9 in that paper matches eq. 6 in your paper, leading me to believe that it may be the code which is incorrect.

The way the code is currently being called rho was set at 1 so that this is not affecting the results for now, but would affect the results if rho were changed when calling ADMMSolver:

TICC/TICC_solver.py

Lines 334 to 335 in 5788a14

    
           rho = 1 
        
           solver = ADMMSolver(lamb, self.window_size, size_blocks, 1, S)

Thanks so much for pointing this out! It turns out that you found an even bigger issue than you initially thought... Following the reference you included, you can see the that original TICC paper actually was incorrect, having a small typo (we replaced rho with 1/rho in 2 places in the \theta update step). Though we have always just used rho = 1, which means the 2 values are identical in practice. Either way, though, thanks for helping us realize this typo!

Regarding the solver, the main change is that eta (which was 1/rho) should actually be just equal to rho, then the rest of the Prox_LogDet step is correct. I just pushed that one-line change, so the new solver should be correct now.

Once again, thanks so much for catching this, and hopefully TICC should be working as desired now!

	rho = 1
	solver = ADMMSolver(lamb, self.window_size, size_blocks, 1, S)