[MRG] cleaned tests #20

QB3 · 2019-11-21T10:51:23Z

solves #4

codecov-io · 2019-11-21T13:22:15Z

Codecov Report

Merging #20 into master will increase coverage by 20.8%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   65.78%   86.59%   +20.8%     
==========================================
  Files           9        6       -3     
  Lines         681      552     -129     
==========================================
+ Hits          448      478      +30     
+ Misses        233       74     -159

Impacted Files	Coverage Δ
clar/duality_gap.py	`100% <ø> (+66.66%)`	⬆️
clar/utils.py	`55.43% <ø> (+15.56%)`	⬆️
clar/tests/test_solvers.py	`100% <100%> (ø)`
clar/data/artificial.py	`96.29% <66.66%> (+24.23%)`	⬆️
clar/solvers.py	`86.75% <88.88%> (+8.49%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fb53df...d93b9bd. Read the comment docs.

QB3 · 2019-11-21T13:27:44Z

ready for merging IMO

QB3 · 2019-11-21T13:41:40Z

I think this is ready to merge @mathurinm

mathurinm · 2019-11-21T14:12:29Z

clar/tests/test_solvers.py

+    B_mtl, _, E, gaps = solver(
+        X, Y, alpha, sigma_min, B0=None,
+        tol=tol, pb_name=pb_name, n_iter=10000)
+    gap = gaps[-1]


@QB3 you can directly assert using gaps[-1]

mathurinm · 2019-11-21T14:12:46Z

clar/tests/test_solvers.py

+        X, Y, alpha, sigma_min, B0=B_mtl,
+        tol=tol, pb_name=pb_name, n_iter=10000)
+    np.testing.assert_equal(len(E), 2)
+    gap = gaps[-1]


mathurinm · 2019-11-21T14:13:13Z

clar/tests/test_solvers.py

+    tol = 1e-7
+
+    X, all_epochs, _, _ = get_data_me(
+        dictionary_type="Gaussian", noise_type="Gaussian_iid",


maybe remove those which are default values

mathurinm · 2019-11-21T14:13:28Z

clar/tests/test_solvers.py

+    sigma_min = get_sigma_min(Y)
+    alpha_max = get_alpha_max(X, Y, sigma_min, "SGCL")
+    alpha = alpha_max * 0.9
+    print("alpha = %.2e" % alpha)


mathurinm · 2019-11-21T14:14:23Z

clar/tests/test_solvers.py

+    log_gap = np.log10(gaps[-1])
+    log_gap_accel = np.log10(gaps_accel[-1])
+    np.testing.assert_array_less(
+        np.minimum(log_gap, log_gap_accel), np.log10(tol) * E[0])


why taking log everywhere?

mathurinm · 2019-11-21T14:25:03Z

clar/tests/test_solvers.py

+        n_iter=10**4, tol=10**-4)[0]
+
+    old_size_supp = 0
+    for supp in dict_masks.values():


what are we testing here?

that the size of the support is growing.
what would you like to test ?

Although it's a valid intuition, we can't say anything if the test breaks: there is no proof that support have growing sizes

mathurinm · 2019-11-21T14:25:26Z

clar/tests/test_solvers.py

+        tol=tol, pb_name=pb_name, n_iter=1000,
+        alpha_Sigma_inv=alpha_Sigma_inv)[-2]
+
+    assert (Es[-1] - Es[-2]) <= 1e-10


mathurinm · 2019-11-21T14:25:56Z

clar/tests/test_solvers.py

+    alpha_Sigma_inv = 0.01
+
+    Y = np.mean(all_epochs, axis=0)
+    sigma_min = get_sigma_min(Y)


can you check that duality gap is 0 for alpha max and solver returns 0?

mathurinm · 2019-11-21T15:08:17Z

clar/data/artificial.py

@@ -185,19 +185,3 @@ def get_toeplitz_dictionary(
    covar = toeplitz(vect, vect)


you can fix this too

mathurinm · 2019-11-21T15:08:58Z

clar/tests/test_solvers.py

+    _, _, E, gaps = solver(
+        X, Y, alpha, sigma_min, B0=B_mtl,
+        tol=tol, pb_name=pb_name, n_iter=10000)
+    np.testing.assert_equal(len(E), 2)


is it normal that convergence happens so fast?

it seems fair since we are passing B0=B_mtl with the same arguments ?

mathurinm · 2019-11-21T15:09:31Z

clar/tests/test_solvers.py

+        n_sources=n_sources, n_active=3, rho_noise=rho_noise,
+        SNR=SNR)
+
+    Y = np.mean(all_epochs, axis=0)


can we avoid defining this variable?

mathurinm · 2019-11-21T15:09:47Z

clar/tests/test_solvers.py

+
+    alpha_max = get_alpha_max(X, all_epochs, sigma_min, pb_name=pb_name)
+
+    alpha_div = 1.1


maybe it's better to test on a more difficult problem ?

mathurinm · 2019-11-21T15:10:05Z

clar/tests/test_solvers.py

+    alpha_max = get_alpha_max(X, all_epochs, sigma_min, pb_name=pb_name)
+
+    alpha_div = 1.1
+    alpha = alpha_max / alpha_div


directly divide by the value you want ? (say 20 ?)

mathurinm · 2019-11-21T15:10:27Z

clar/tests/test_solvers.py

+    Y = all_epochs.mean(axis=0)
+    sigma_min = get_sigma_min(Y)
+    alpha_max = get_alpha_max(X, Y, sigma_min, "SGCL")
+    alpha = alpha_max * 0.9


again a very easy problem

mathurinm · 2019-11-22T07:18:02Z

clar/tests/test_solvers.py

+
+    B_mtl, _, E, gaps = solver(
+        X, Y, alpha, sigma_min, B0=None,
+        tol=tol, pb_name=pb_name, n_iter=10000)


maybe rm n_iter?

mathurinm · 2019-11-22T07:18:20Z

clar/tests/test_solvers.py

+    alpha = alpha_max / alpha_div
+
+    B_mtl, _, E, gaps = solver(
+        X, Y, alpha, sigma_min, B0=None,


B0 = None is default so not needed

mathurinm · 2019-11-22T07:19:06Z

clar/tests/test_solvers.py

+    all_epochs[0] = Y
+
+    _, _, E, (gaps, gaps_accel) = solver(
+        X, Y, alpha, sigma_min, B0=None, n_iter=n_iter,


rm paramters having their default value

mathurinm · 2020-01-09T16:30:44Z

One last pass @QB3 :) !

mathurinm · 2020-01-21T09:08:39Z

Good to merge @QB3 ?

qbertrand added 4 commits November 21, 2019 11:53

cleaned tests 1

e1bd445

remove jit in duality gap

3d9da12

reshape tests + remove unused functions

b5bfbb5

added test for computing solver along a path

f405a75

cosmit

aeb4da4

QB3 changed the title ~~cleaned tests 1~~ [MRG] cleaned tests Nov 21, 2019

mathurinm reviewed Nov 21, 2019

View reviewed changes

pass on test thx to Mathurin's comments

7a2def0

mathurinm reviewed Nov 21, 2019

View reviewed changes

removed wierd log comparison, now compare gaps instead

a81c9a2

mathurinm reviewed Nov 21, 2019

View reviewed changes

qbertrand added 2 commits November 21, 2019 16:40

new round of modif thx to Mathurin's comments

8545e1a

remove flake8 error

d93b9bd

mathurinm reviewed Nov 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] cleaned tests #20

[MRG] cleaned tests #20

QB3 commented Nov 21, 2019

codecov-io commented Nov 21, 2019 •

edited

Loading

QB3 commented Nov 21, 2019

QB3 commented Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

QB3 Nov 21, 2019

mathurinm Nov 22, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

QB3 Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 21, 2019

mathurinm Nov 22, 2019

mathurinm Nov 22, 2019

mathurinm Nov 22, 2019

mathurinm commented Jan 9, 2020

mathurinm commented Jan 21, 2020

		@@ -185,19 +185,3 @@ def get_toeplitz_dictionary(
		covar = toeplitz(vect, vect)


		alpha_max = get_alpha_max(X, all_epochs, sigma_min, pb_name=pb_name)

		alpha_div = 1.1

[MRG] cleaned tests #20

Are you sure you want to change the base?

[MRG] cleaned tests #20

Conversation

QB3 commented Nov 21, 2019

codecov-io commented Nov 21, 2019 • edited Loading

Codecov Report

QB3 commented Nov 21, 2019

QB3 commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathurinm commented Jan 9, 2020

mathurinm commented Jan 21, 2020

codecov-io commented Nov 21, 2019 •

edited

Loading