-
Notifications
You must be signed in to change notification settings - Fork 12
Description
The actual tests for s_equi is a shadow test and needs to be improved.
hidimstat/test/test_knockoff.py
Line 293 in 319b433
| def test_s_equi_not_define_positive(): |
- First improvement: Generate a symmetric matrix
hidimstat/test/test_knockoff.py
Lines 308 to 312 in 319b433
# matrix with positive eigenvalues, positive diagonal while not np.all(np.linalg.eigvalsh(a) > tol): a += 0.1 * np.eye(n) with pytest.warns(UserWarning, match="The equi-correlated matrix"): _s_equi(a)
A is not symmetric... is it purposeful ?
Originally posted by @bthirion in #366
Why A should be symmetric?
Originally posted by @lionelkusch in #366
It's supposed to be a covariance matrix, right ?
Originally posted by @bthirion in #366
It should be a correlation matrix but yes.
However, this PR is not about improving these tests. These tests were just moved from one file to another one. It's about the Gaussian Sampler.
Originally posted by @lionelkusch in #366
OK, but let's make A symmetric:
A = (A + A.T) / 2
Originally posted by @bthirion in #366
- The second improvement is to change the method to generate a positive definite matrix:
hidimstat/test/test_knockoff.py
Lines 315 to 317 in 319b433
u, s, vh = np.linalg.svd(a) d = np.eye(n) sigma = u * d * u.T
With this formula, sigma is the identity...
Originally posted by @bthirion in #366
You proposed to me this formula for making a positive define matrix.
In this case, sigma is a diagonal matrix and not the identity.
If you have a better proposition, we can change it but I will be better at it in other PR because it's not the aim of this one.
Originally posted by @lionelkusch in #366
I d is eye(n), then sigma will be the identity. If you want sigma to be a general sdp matric, rather use:
sigma = u * s * u.T
after making sure that s doe not contain any 0.
Originally posted by @bthirion in #366
- The third improvement: add an assertion.
A first attempt is made in PR Reformat the gaussian conditional sampler #366.
np.testing.assert_equal(res / np.diag(sigma), np.ones_like(res))
here the assertion is trivially true.