[API 2]: Desparsified Lasso by lionelkusch · Pull Request #381 · mind-inria/hidimstat

lionelkusch · 2025-09-05T11:44:39Z

No description provided.

codecov · 2025-09-08T14:32:02Z

Codecov Report

❌ Patch coverage is 97.84483% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.94%. Comparing base (f1cf295) to head (48f28bc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/hidimstat/desparsified_lasso.py	98.04%	4 Missing ⚠️
src/hidimstat/ensemble_clustered_inference.py	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
- Coverage   99.19%   98.94%   -0.25%     
==========================================
  Files          24       23       -1     
  Lines        1364     1424      +60     
==========================================
+ Hits         1353     1409      +56     
- Misses         11       15       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bthirion

Looks good overall. I left a few suggestions.

jpaillard · 2025-09-10T08:55:55Z

+                clf=clone(self.lasso).set_params(
+                    alpha=alphas[i],
+                    precompute=np.delete(np.delete(gram, i, axis=0), i, axis=1),
+                    random_state=np.random.RandomState(
+                        np.random.default_rng(streams[i]).bit_generator
+                    ),


It will not be intuitive for the user that the parameters of the model are overwritten inside the method. IMO, the model's parameter should be left unchanged. The _alpha_max trick could be shown in an example as a way to find the list of alphas, but not hidden in the function.

@bthirion What do you think?

I don't see the problem with working with a given random generator or seed, since the problems are different.
I agree that we should not hide parameter settings in the functions/methods.

I added an option to change alphas.

jpaillard · 2025-10-31T14:57:07Z

I made a pass to solve the last discussion points.

I also added an example using EnCluDL on the MNIST dataset. By writing it, I got a bit confused by the output of ensemble_clustered_inference_pvalue. The docstring indicates that the p-values, derived from a two-sided test on the DL coefficients $\hat\beta$, have a sign corresponding to the sign of $\hat\beta$. However, as illustrated in the example, what is computed is not very clear.

bthirion

There is mostly the one_minus_pvalue_ thing to fix. It's looking good otherwise.

bthirion · 2025-11-04T22:30:59Z

 selected_dl = np.logical_or(
-    pval_corr < fwer_target / 2, one_minus_pval_corr < fwer_target / 2
+    desparsified_lasso.pvalues_corr_ < fwer_target / 2,
+    1 - desparsified_lasso.pvalues_corr_ < fwer_target / 2,


No this is incorrect I'm afraid, because due to the correction one_minus_pval_corr is not 1 - desparsified_lasso.pvalues_corr_

bthirion · 2025-11-04T22:34:20Z

+)
+
+beta_hat_1_7, selected_ecdl_1_7 = ensemble_clustered_inference_pvalue(
+    X_1_7.shape[0], False, ward_1_7, desparsified_lassos_1_7, fdr=fdr


I'd rather use fwer control rather than fdr control. I think it makes more sense for EncluDL

bthirion · 2025-11-04T22:34:53Z

+# %%
+# References
+# ----------
+# TODO


can you add it ?

bthirion · 2025-11-04T22:42:32Z

    interior_support = support_size - margin_size
    extended_support = support_size + margin_size
    n_bootstraps = 4
+    fdr = 0.3


This is a bit lenient as a control.
And we should rather check that FWER control is rigorous.

Is it possible to change this ?

bthirion · 2025-11-04T22:43:48Z

+from hidimstat.ensemble_clustered_inference import ensemble_clustered_inference_pvalue
+
+n_jobs = 5
+fdr = 0.5


sounds too weak: "in average, half of your discoveries are garbage"

…sified_lasso

jpaillard · 2025-11-05T15:15:48Z

I fixed the 1 - pvalues
I removed the "digits" example for now. It's nice to look at the coefficient maps, but the selection is way less clear. The "background cluster" is always selected, probably because the text between images is not aligned. We can discuss it in the PR about EnCluDL
Same for the tests on EnCluDL, I suggest making a pass on all the tests of EnCluDL in the dedicated PR
In addition, it is strange that ensemble_clustered_inference_pvalue actually doesn't return a pvalue but a selection mask. We can also discuss this inthe next PR

bthirion

LGTM, thx. I have 2 relatively minor comments pending.

bthirion · 2025-11-05T22:24:05Z

    interior_support = support_size - margin_size
    extended_support = support_size + margin_size
    n_bootstraps = 4
+    fdr = 0.3


Is it possible to change this ?

jpaillard · 2025-11-06T08:42:34Z

Done.
I also added the reproducibility/randomness tests for consistency, as stated in the documentation

bthirion

LGT

lionelkusch added 4 commits September 4, 2025 21:09

update Desparsified Lasso

4b7abd6

fix desparsified lasso and the example

ac987d8

fix example

bfa5718

Merge branch 'main' into PR_desparsified_lasso

4bcca60

add tests

fc1f941

lionelkusch marked this pull request as ready for review September 8, 2025 15:37

update docstring

e4d1c44

bthirion reviewed Sep 9, 2025

View reviewed changes

lionelkusch added the API 2 label Sep 9, 2025

lionelkusch added 2 commits September 9, 2025 16:43

change signal noise ratio

076148e

change name for model

f28126c

jpaillard reviewed Sep 10, 2025

View reviewed changes

smal modification

d8c0f80

lionelkusch mentioned this pull request Sep 10, 2025

use the check_fit of scikitlearn #385

Open

lionelkusch added 15 commits September 10, 2025 11:42

fix name variable

631cf87

fix docstring

b54fe16

fix example and test

4135740

Merge branch 'main' into PR_desparsified_lasso

fc90820

Merge branch 'main' into PR_desparsified_lasso

60e7a59

fix commit

571e306

rename variable

af12949

fix format

fae39ee

fix example

cc1a85a

update docstring

3666fdf

add warning

7034a3b

add warning

283e760

fix docstring

7ab50a7

fix docstring

a008b89

Merge branch 'main' into PR_desparsified_lasso

ac0291d

lionelkusch linked an issue Oct 27, 2025 that may be closed by this pull request

desparsified functions #60

Closed

lionelkusch mentioned this pull request Oct 27, 2025

Move the code of "desparsified_lasso_pvalue" to a more generic function in stats_tools #185

Open

jpaillard added 2 commits October 30, 2025 09:32

Merge branch 'main' into PR_desparsified_lasso

14e79b6

pass Desparsified Lasso

54541d0

jpaillard changed the title ~~API 2: Desparsified Lasso~~ [API 2]: Desparsified Lasso Oct 30, 2025

jpaillard added 6 commits October 31, 2025 09:25

fix tests

dcf8a17

fix tests

036e377

add example

19f86ea

fix example

b68c9a6

text in example

f5b9fbb

small rendering fix

c9c08b8

jpaillard requested a review from bthirion October 31, 2025 17:04

bthirion reviewed Nov 4, 2025

View reviewed changes

jpaillard added 4 commits November 5, 2025 09:38

fix 1- pval, place imports along the exemple

5df7264

Merge branch 'main' of github.com:mind-inria/hidimstat into PR_despar…

e043c8f

…sified_lasso

finish merge

d6a7765

remove digits example

14c43ce

jpaillard requested a review from bthirion November 5, 2025 15:15

bthirion reviewed Nov 5, 2025

View reviewed changes

jpaillard added 2 commits November 6, 2025 08:58

add docstring

e4d9270

fix FDR in test, add randomness tests for DL

408fe0e

jpaillard requested a review from bthirion November 6, 2025 08:48

bthirion approved these changes Nov 6, 2025

View reviewed changes

Merge branch 'main' into PR_desparsified_lasso

48f28bc

jpaillard merged commit 1f97f5b into mind-inria:main Nov 7, 2025
23 of 24 checks passed

jpaillard deleted the PR_desparsified_lasso branch November 7, 2025 08:33

jpaillard mentioned this pull request Dec 8, 2025

Release 0.3.0 #551

Merged

Conversation

lionelkusch commented Sep 5, 2025

Uh oh!

codecov Bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bthirion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpaillard commented Oct 31, 2025

Uh oh!

bthirion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jpaillard commented Nov 5, 2025

Uh oh!

bthirion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpaillard commented Nov 6, 2025

Uh oh!

bthirion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 8, 2025 •

edited

Loading