Policie of test: How to test randomize methods?

[lionelkusch]

The PR #265 introduces a way of testing the different methods.
However based on the discussion in PR #368, it seems that the chosen way to test the different methods is not correct.
Moreover, there are already some issues with the creation of tests for reid function (issue #336) and for desparsified lasso (issue #311).

We need to make decisions about what we want to test and why.

[lionelkusch]

I just put back the discussion of the PR #368, which want to modified the following line:

hidimstat/test/test_conditional_feature_importance.py

Lines 76 to 81 in bafa4ed

	parameter_exact = [
	("HiDim", 150, 200, 10, 0.0, 42, 1.0, np.inf, 0.0),
	("HiDim with noise", 150, 200, 10, 0.0, 42, 1.0, 10.0, 0.0),
	("HiDim with correlated noise", 150, 200, 10, 0.0, 42, 1.0, 10.0, 0.2),
	("HiDim with correlated features", 150, 200, 10, 0.2, 42, 1.0, np.inf, 0.0),
	]

test/test_conditional_feature_importance.py
("HiDim", 150, 10, 5, 0.0, 42, 1.0, np.inf, 0.0),
("HiDim with noise", 150, 10, 5, 0.0, 42, 1.0, 10.0, 0.0),
("HiDim with correlated noise", 150, 10, 5, 0.0, 42, 1.0, 10.0, 0.2),
("HiDim with correlated features", 150, 10, 5, 0.2, 42, 1.0, np.inf, 0.0),

_Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

Had to change the test to make it pass. I think it was too hard before: using n > p, and probably passed thanks to lucky seeding.

_Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

It was not the case.
CFI should handle high-dimensional data.

_Originally posted by @lionelkusch in #368 (comment)

[lionelkusch]

I returned to the previous test but allowed for a small tolerance: Asking for a perfect support recovery with n=150 p=200, with correlation and noise is ambitious.
I test for the 10 important variables to be in the top 15 in importance returned by CFI.

_Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

I made 3 categories of test:

• exact: which returns all the important variables
• partial: which returns all a part of it
• failed: which doesn't succeed in finding them

I would like to keep this spirit for all the tests.

_Originally posted by @lionelkusch in #368 (comment)

[lionelkusch]

CFI is probabilistic by nature; there are no guarantees that it will identify exactly the support 100% of the time.
In addition, several factors make it even harder in the "exact" test:

1. n < p with p=150!
2. No CV is used for aggregating the importances over folds
3. Only 10 permutations are used
4. There is noise and correlation in the data

I think the "exact" test is unrealistic.

_Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

It depends on what exactly you want to test, but the straightforward solution is to make the simulation simple enough that it always work. Do you have any particular test in mind ?

_Originally posted by bthirion in #368 (comment)

[lionelkusch]

CFI is probabilistic by nature; there are no guarantees that it will identify exactly the support 100% of the time. In addition, several factors make it even harder in the "exact" test:

1. n < p with p=150!

2. No CV is used for aggregating the importances over folds

3. Only 10 permutations are used

4. There is noise and correlation in the data

I think the "exact" test is unrealistic.

From my tests, the exact tests were working and it's flexible enough to accept a >range of parameters but strict enough to detect any substantiation difference in >behaviour of the method.
If you see my last comment, you can see that there is an important bug to fix.

_Originally posted by @lionelkusch in #368 (comment)

[lionelkusch]

I do not challenge that the test is flexible and accepts a range of parameters.
What I try to say is that it is too strict and may fail because of the natural randomness of CFI. For instance, it fails if I try to run the test_linear_data_exact of main but change the seed from 0 to 3.

That's why one solution is to allow some tolerance to errors, for instance, check that the 10 important features are in the top 15 returned by CFI:
np.all([int(i) in np.argsort(importance)[-15:] for i in important_features])
Or, as @bthirion suggests, reduce p, which would also limit the computation cost of the CI.

_Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

I tested a seed between 0 and 9. There is the test "HiDim with correlated noise" which failed for seeds 3, 4, 7, 9.
I don't see why there is a problem of being too strict if we fix a seed.
The CI is here to check if there is a modification in the behaviour. I don't see the point of testing exact values.
As I mentioned without this test, you didn't detect the bug that I mentioned.

Originally posted by @lionelkusch in #368 (comment)

[lionelkusch]

It's a question of what we call a modification of the behavior. IMO, modifying the seeding (done in this PR) of the method (done in this PR) does not change its behavior, and tests should not be sensitive to this. Otherwise, a simple change in numpy's generator, completely independent of the method, could make the test fail.

Originally posted by @jpaillard in #368 (comment)

[lionelkusch]

I will agree if it's required a very specific seed. In this case, there is only 1 test which fails for less than half of the seeds.
In my opinion, the percentage of 50% seed is a good threshold to declare a test that is too sensible or not.

Originally posted by @lionelkusch in #368 (comment)