Permutation_test (1/4): add comments and docstring of the functions #111

lionelkusch · 2025-01-02T15:08:45Z

I removed the function permutation_test_cv. This function was a permutation_test with cross-validation for finding the best parameter C for the linearSVR. In this context, it can be used as a global surrogate based on Linear SVR but I estimated it was too specific and that the user can do it by hand for the moment.
I move the function step_down_max_t to stat_tool because it's a function used for computing the pvalue.
I modified the function permutation_test in consequence for returning the weight and the distribution of the weights from the permutation of y. I think that this intermediate step is more adapted to a general API.
I modified the test in consequence with this new signature of functions.

lionelkusch · 2025-01-02T15:10:37Z

My comments for step_down_max_t need to be verified because there are based on AI.
I don't perfectly the algorithm but the actual code seems very different to the original code.
Can someone check it?

codecov · 2025-01-02T15:41:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f9ff6b9) to head (6e92b7a).
Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##             main      #111       +/-   ##
============================================
+ Coverage   84.25%   100.00%   +15.74%     
============================================
  Files          43        21       -22     
  Lines        2452       889     -1563     
============================================
- Hits         2066       889     -1177     
+ Misses        386         0      -386

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

bthirion

The PR looks mostly good. I have 2 comments:

I don't see why we no longer allow to run CV
the Westfall-Young "maxT" procedure seems overly complicated

src/hidimstat/stat_tools.py

src/hidimstat/permutation_test.py

jpaillard

To re-open the discussion on the API: when I re-implemented the PermutationImportance class (which differs since X columns are permuted instead of y here), I found it convenient to have a class with .fit and .score, rather than a function like here.

Would having consistency in having both as functions or classes be clearer? If so, I am open to returning to functions if you find it better.
Here again, a difference is that this function does not require train / test split which might be the criterion for using classes.

src/hidimstat/permutation_test.py

jpaillard · 2025-01-03T21:11:20Z

examples/plot_fmri_data_example.py

@@ -152,18 +153,23 @@ def preprocess_haxby(subject=2, memory=None):
 SVR_permutation_test_inference = False
 if SVR_permutation_test_inference:
    # We computed the regularization parameter by CV (C = 0.1)
-    pval_corr_svr_perm_test, one_minus_pval_corr_svr_perm_test = permutation_test_cv(
-        X, y, n_permutations=50, C=0.1
+    estimator = LinearSVR(C=0.1)


C is the regularization parameter, it is not optimized via CV here. Or am I missing something?
I suggest using something like randomized search.

Suggested change

estimator = LinearSVR(C=0.1)

estimator = RandomizedSearchCV(

LinearSVR(random_state=42),

param_distributions={ "C": np.logspace(-3, 3, 10), },

n_iter=10,

n_jobs=5,

random_state=42,

)

I didn't provide an optimisation by CV because, in the original example, it wasn't the case.
Nevertheless, we need to be careful with optimisation because it will increase the time for running examples.

Do you have an estimate of the compute time ?

On my computer, the example without the CV is: 5m39s and with CV is: 7m16s.
In estimation, it increases 2 minutes the time of calculation.

One solution can be to store the best value of the parameter and avoid doing the refitting each time. .

Also, there's L153 SVR_permutation_test_inference = False so is this even running in the CI?
If not, I suggest leaving the CV to show this possibility to the user without increasing CI time.

For the time being, we should not change this if there is no explicit reason for that (e.g. significant reduction of documentation generation time).

My feeling is that this is toio much time for a method that does not enjoy any theoretical guarantee. Out of curiosity, what do you get if you replace the SVR with a RidgeRegression ?

This is already done in the example.

Please use a RidgeCV, so that there is no magic alpha parameter hanging around.
@bthirion Do we do the cross-validation for Ridge and linearSVR or only for Ridge?

examples/plot_fmri_data_example.py

src/hidimstat/permutation_test.py

lionelkusch · 2025-01-31T09:13:55Z

@jpaillard

bthirion

Thx for this work. Minor details pending.

docs/_downloads/06bb0f682e15138dfd659b791a14e9c6/plot_variable_importance_classif.zip

docs/_sources/auto_examples/plot_2D_simulation_example.rst.txt

docs/_sources/auto_examples/plot_fmri_data_example.rst.txt

docs/auto_examples/plot_knockoff_aggregation.html

examples/plot_fmri_data_example.py

src/hidimstat/permutation_test.py

src/hidimstat/stat_tools.py

test/test_permutation_test.py

bthirion

2 minor things, and then it's good to go !

bthirion · 2025-02-04T20:05:35Z

examples/plot_fmri_data_example.py

-estimator = Ridge()
-pval_corr_ridge_perm_test, one_minus_pval_corr_ridge_perm_test = permutation_test(
+# We computed the parameter from a cross valisation (alpha = 0.0215)
+estimator = Ridge(alpha=0.0215)


Please use a RidgeCV, so that there is no magic alpha parameter hanging around.

See comment above.

Sorry, I didn't get the point. How much more time does RidgeCV take ?
If you want to have reasonable computation time, then we should dimply get rid of the SVR (especially because we use hard-coded C parameter, which is unclean).

Cross-validation of LinearSVR: 56s
Instantiation of LinearSVR: >1ms
Permutation test of LinearSVR: 22O s

Permutation test with RidgeCV: 81.6 s
Permutation test with Ridge: 10.7 s

Then another possibility would be to call RidgeCV once to learn the hyperparameter and then plug it into the Permutation test. Does this make computation time more reasonable ?

Not exactly. RidgeCV uses an arbitrary grid that may not contains the correct range of values.
When you use Ridge, you can change the regularizer by a factor of 2 or so, this has limited impact. What you need is just to pick the right magnitude order.
The default value of alpha may not be of the right order of magnitude (?)
Permuting y only has a big impact on the optimal alpha of the variance fit by the model is large (e.g. 90%) : I guess it is not the case here ?
As you started the example a while ago with an alpha close to 0.02, can you confirm using grid search that this is the value to pick ?

For alpha equals 1.0, the default value, the algorithm converges for every permutation.
For alpha equals 0.01, the value of the cross-validation based on the data, the algorithm doesn't converge for some permutation.
I forget how I got the value of 0.02, I probably changed the range of value of the CV but with this alpha I didn't get any warnings.
I look in detail what are the values of alpha by using RidgeCV, I find that applying the CV on the data provides a value of 0.1 for alpha and apply the CV on data with a permutation of y, I get a value of alpha equals 10.0. As I expected, the shuffle of alpha changes the optimal alpha. In this case, the change is in 3 orders of magnitude.
What value do you want to pick for having the right magnitude?

OK, then the question becomes: does it make a true difference in terms of results to use Ridge vs RidgeCV ?
If yes, I think that we should rely on RidgeCV.

Using Ridge or RidgeCV in this example provides the same result; there are not significant voxels.

Ha, ha. Then I'd go with defaults parameters, without CV, and add a comment stating that ideally you want to use a RidgeCV, but this is not done here for the sake of time.

test/test_permutation_test.py

lionelkusch added 9 commits January 2, 2025 15:59

Fix gitignore

ac17884

Remove not necesaru function

65b5564

refactor files

ac8b76f

Add option to pytest for detection of test

ca0b257

Fix test

1d81370

Remove not necessary normalisation

316a245

Comment step_down_maxt algo

1e1badf

Modify function of the integrate the difference.

4f98a49

format stat tools

5a29cdc

lionelkusch added 5 commits January 2, 2025 16:23

add formating

7aaa78f

Fixe bug

0ccb7ff

Format files

7322a3b

Fix a bug from the reformating

87bb15c

Better function for empty matrix

75048fc

lionelkusch requested review from jpaillard and bthirion January 2, 2025 15:42

lionelkusch added 4 commits January 2, 2025 16:50

Add a citation

3a271b8

Fix format

df483d1

Improve actual tests

62d1142

Add two tests

f8b2a7f

bthirion reviewed Jan 3, 2025

View reviewed changes

src/hidimstat/stat_tools.py Show resolved Hide resolved

src/hidimstat/permutation_test.py Show resolved Hide resolved

jpaillard reviewed Jan 3, 2025

View reviewed changes

lionelkusch added 4 commits January 7, 2025 15:39

format type

bea81fe

Merge branch 'main' into PR_permutation_test

e3fc566

change default values

d39bed9

Format file

8d19b65

jpaillard reviewed Jan 20, 2025

View reviewed changes

src/hidimstat/permutation_test.py Outdated Show resolved Hide resolved

Change default argument for parallelization

47b7d84

jpaillard approved these changes Feb 3, 2025

View reviewed changes

Update example

fd8e475

bthirion reviewed Feb 4, 2025

View reviewed changes

Merge branch 'main' into PR_permutation_test

0299315

lionelkusch force-pushed the PR_permutation_test branch from bd6debe to 0299315 Compare February 4, 2025 08:49

lionelkusch added 3 commits February 4, 2025 10:47

include all

d785bd1

Modification folowwing suggestion

72ca7d5

Format file

0fb452e

bthirion reviewed Feb 4, 2025

View reviewed changes

Small modification

6e92b7a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permutation_test (1/4): add comments and docstring of the functions #111

Permutation_test (1/4): add comments and docstring of the functions #111

lionelkusch commented Jan 2, 2025

lionelkusch commented Jan 2, 2025

codecov bot commented Jan 2, 2025 •

edited

Loading

bthirion left a comment

jpaillard left a comment

jpaillard Jan 3, 2025

lionelkusch Jan 6, 2025

bthirion Jan 6, 2025

lionelkusch Jan 7, 2025

jpaillard Jan 7, 2025

lionelkusch Jan 31, 2025

bthirion Jan 31, 2025

lionelkusch Feb 3, 2025

lionelkusch Feb 5, 2025

lionelkusch Feb 7, 2025

lionelkusch commented Jan 31, 2025

bthirion left a comment

bthirion left a comment

bthirion Feb 4, 2025

lionelkusch Feb 5, 2025

bthirion Feb 5, 2025

lionelkusch Feb 10, 2025

bthirion Feb 11, 2025

bthirion Feb 12, 2025

lionelkusch Feb 13, 2025

bthirion Feb 13, 2025

lionelkusch Feb 14, 2025

bthirion Feb 14, 2025

-    estimator = LinearSVR(C=0.1)
+    estimator = RandomizedSearchCV(
+        LinearSVR(random_state=42),
+        param_distributions={ "C": np.logspace(-3, 3, 10), },
+        n_iter=10,
+        n_jobs=5,
+        random_state=42,
+    )

Permutation_test (1/4): add comments and docstring of the functions #111

Are you sure you want to change the base?

Permutation_test (1/4): add comments and docstring of the functions #111

Conversation

lionelkusch commented Jan 2, 2025

lionelkusch commented Jan 2, 2025

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

bthirion left a comment

Choose a reason for hiding this comment

jpaillard left a 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

lionelkusch commented Jan 31, 2025

bthirion left a comment

Choose a reason for hiding this comment

bthirion left a 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

codecov bot commented Jan 2, 2025 •

edited

Loading