Rank1 support #48

rprimet · 2021-11-04T10:52:41Z

Very rough draft of part 1 of the implementation of #46 for discussion / comments.

codecov · 2021-11-04T10:57:06Z

Codecov Report

Merging #48 (bb23361) into main (ad30e87) will increase coverage by 1.29%.
The diff coverage is 97.46%.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   74.50%   75.80%   +1.29%     
==========================================
  Files          43       43              
  Lines        2593     2748     +155     
==========================================
+ Hits         1932     2083     +151     
- Misses        661      665       +4

Flag	Coverage Δ
unittests	`75.80% <97.46%> (+1.29%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dicodile/update_z/distributed_sparse_encoder.py	`74.25% <71.42%> (-2.34%)`	⬇️
dicodile/update_z/coordinate_descent.py	`41.17% <80.00%> (-0.63%)`	⬇️
dicodile/update_z/dicod.py	`78.68% <100.00%> (+0.84%)`	⬆️
.../update_z/tests/test_distributed_sparse_encoder.py	`100.00% <100.00%> (ø)`
dicodile/utils/csc.py	`100.00% <100.00%> (ø)`
dicodile/utils/dictionary.py	`51.45% <100.00%> (+19.02%)`	⬆️
dicodile/utils/tests/test_csc.py	`100.00% <100.00%> (ø)`
dicodile/workers/dicod_worker.py	`94.14% <100.00%> (+0.06%)`	⬆️

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 ad30e87...bb23361. Read the comment docs.

tomMoral

a few comments but it seems to be going in the right direction.

I would say for a first draft, lets aim to only use get_D in the worker?

dicodile/update_z/dicod.py

dicodile/update_z/tests/test_distributed_sparse_encoder.py

dicodile/update_z/distributed_sparse_encoder.py

…itely

…g uv_hat

dicodile/update_z/tests/test_distributed_sparse_encoder.py

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

tomMoral

some comments

dicodile/utils/tests/test_csc.py

dicodile/workers/dicod_worker.py

dicodile/utils/tests/test_csc.py

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

…use a tuple

tomMoral

Seems right for the compute_DtD!! Nice! 👍

dicodile/utils/csc.py

dicodile/utils/dictionary.py

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

dicodile/update_z/tests/test_distributed_sparse_encoder.py

tomMoral

Ok I think we are nearly there!
A few extra comments.

dicodile/_dicodile.py

dicodile/update_z/distributed_sparse_encoder.py

dicodile/update_z/tests/test_distributed_sparse_encoder.py

dicodile/utils/csc.py

dicodile/utils/dictionary.py

dicodile/utils/tests/test_csc.py

dicodile/workers/dicod_worker.py

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

dicodile/utils/dictionary.py

…e_convolve

tomMoral

A small comments on why the test is breaking.

dicodile/utils/csc.py

agramfort

@tomMoral merge if happy

tomMoral · 2021-12-20T15:19:24Z

The example is nice and working! LGTM! thanks @rprimet

agramfort · 2021-12-20T16:06:04Z

🙏 @rprimet !

rprimet added 3 commits October 21, 2021 16:59

drafting rank1 support

c99c008

add rank1 case in recv_D

01adda9

draft part one of the rank1 support in dicodile

662c997

assert for uv shape

cff32c9

tomMoral reviewed Nov 4, 2021

View reviewed changes

rprimet added 4 commits November 5, 2021 10:23

retrieve n_channels from the shape of X rather than passing it explic…

042de1e

…itely

reordering

07cacb0

avoid recomputing the full dict in DistributedSparseEncoder when usin…

c9fda84

…g uv_hat

add D ndim assert

b6eee3e

tomMoral reviewed Nov 6, 2021

View reviewed changes

dicodile/update_z/tests/test_distributed_sparse_encoder.py Outdated Show resolved Hide resolved

rprimet and others added 6 commits November 9, 2021 15:05

Update dicodile/update_z/tests/test_distributed_sparse_encoder.py

46ab2f8

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

DRAFT some interesting funcs for rank_1

2880583

[skip ci] define _is_rank1 function, update _choose_convolve_multi doc

5d94312

fix Xi shape in _dense_convolve_multi_uv

fe35672

[skip ci] add shape test for dense_convolve_multi_uv

91f63b8

[skip ci] fix _dense_convolve_multi_uv and associated test

88a29cf

tomMoral reviewed Nov 16, 2021

View reviewed changes

dicodile/utils/tests/test_csc.py Show resolved Hide resolved

dicodile/workers/dicod_worker.py Outdated Show resolved Hide resolved

test convolve_multi_uv_shape for signal dimensions 1 to 3

1057ca8

tomMoral reviewed Nov 17, 2021

View reviewed changes

dicodile/utils/tests/test_csc.py Outdated Show resolved Hide resolved

rprimet and others added 7 commits November 17, 2021 11:36

Update dicodile/utils/tests/test_csc.py

7b6b7d4

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

[skip ci] add test_convolve_uv_and_convolve_d_match, change get_D to …

22ed789

…use a tuple

fix test

b73c27d

tests passing

f807aaf

use D type rather than passing rank1 explicitely

94fac57

dense_transpose_convolve

c31ab8b

compute_DtD implementation for rank-1 matrices

cd9c13d

tomMoral approved these changes Nov 24, 2021

View reviewed changes

dicodile/utils/csc.py Outdated Show resolved Hide resolved

dicodile/utils/dictionary.py Outdated Show resolved Hide resolved

rprimet and others added 2 commits November 24, 2021 11:37

Update dicodile/utils/dictionary.py

ae0c916

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

keep dense_transpose_convolve private

5603295

rprimet commented Nov 24, 2021

View reviewed changes

dicodile/update_z/tests/test_distributed_sparse_encoder.py Outdated Show resolved Hide resolved

move error-condition-related test case to its own test

595301a

rprimet marked this pull request as ready for review November 24, 2021 14:46

tomMoral approved these changes Nov 24, 2021

View reviewed changes

rprimet and others added 5 commits November 25, 2021 10:21

clearer _d_shape_from_uv

21af6fb

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>

put back comment that was mistakenly removed

d99408f

D shape assert when rank1 is True

410868a

use _choose_convolve_multi to increase coverage

701e5dd

introduce D_shape and compute_norm_atoms_from_DtD

d7d533f

rprimet commented Nov 26, 2021

View reviewed changes

dicodile/utils/dictionary.py Show resolved Hide resolved

rprimet added 3 commits November 29, 2021 09:42

add (failing) equivalence test for rank-1 and generic _dense_transpos…

fca1b03

…e_convolve

reshape atom norms outside of compute_norm_atoms_from_DtD

cb063ad

normalize rank-1 matrix in test_distributed_sparse_encoder

a5e115a

tomMoral reviewed Dec 3, 2021

View reviewed changes

dicodile/utils/csc.py Outdated Show resolved Hide resolved

rprimet added 3 commits December 6, 2021 15:48

fix _dense_transpose_convolve (thanks @tomMoral)

8091d86

toml issue

8ad7971

fix incorrect use of D_shape

bb23361

rprimet mentioned this pull request Dec 16, 2021

Dicodile rank1 example alphacsc/alphacsc#60

Merged

agramfort approved these changes Dec 20, 2021

View reviewed changes

tomMoral merged commit 2588788 into tomMoral:main Dec 20, 2021

tomMoral mentioned this pull request Jan 7, 2022

ENH Implement LGCD with rank1 dictionary #46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rank1 support #48

Rank1 support #48

rprimet commented Nov 4, 2021

codecov bot commented Nov 4, 2021 •

edited

Loading

tomMoral left a comment

tomMoral left a comment

tomMoral left a comment

tomMoral left a comment

tomMoral left a comment

agramfort left a comment

tomMoral commented Dec 20, 2021

agramfort commented Dec 20, 2021

Rank1 support #48

Rank1 support #48

Conversation

rprimet commented Nov 4, 2021

codecov bot commented Nov 4, 2021 • edited Loading

Codecov Report

tomMoral left a comment

Choose a reason for hiding this comment

tomMoral left a comment

Choose a reason for hiding this comment

tomMoral left a comment

Choose a reason for hiding this comment

tomMoral left a comment

Choose a reason for hiding this comment

tomMoral left a comment

Choose a reason for hiding this comment

agramfort left a comment

Choose a reason for hiding this comment

tomMoral commented Dec 20, 2021

agramfort commented Dec 20, 2021

codecov bot commented Nov 4, 2021 •

edited

Loading