[API 2]: CFI, PFI, LOCO

[lionelkusch]

Update the model of CFI, PFI and LOCO for API 2.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.06%. Comparing base (1f97f5b) to head (eca0c0c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   98.94%   99.06%   +0.12%     
==========================================
  Files          23       21       -2     
  Lines        1424     1393      -31     
==========================================
- Hits         1409     1380      -29     
+ Misses         15       13       -2     

☔ 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.

[jpaillard]

It looks good but the diff seems very large for this small change.
Is there a reason for all the other modifications?

[lionelkusch]

I reorganize a bit the parameter in the init and move the docstring to the class because in all the other classes, I plan to do this.

By looking into more details, I miss some parts being added. I will add it and ask you to review it after. Sorry for it.

[bthirion]

This PR is definitely an improvement, thx.

[bthirion]

Thx for the progress. Please find a few suggestions enclosed.

[bthirion]

No further comment than those raised by @jpaillard

[jpaillard]

Regarding the statistical test, the current issue is that the cross-validation scheme has not yet been implemented. Currently, the statistical test is performed using a single train/test split. In that case, when considering the loss values for each individual sample of the test set, they can be considered as independent and we could use ttest instead of nb-ttest as the default.

The NB-t-test actually needs to be the default when CV is used, and losses over test sets cannot be considered as independent.

The comment regarding the nb-ttest implementation remains valid; however, the test fraction shouldn't be hardcoded. I will follow up on #449

[jpaillard]

• I made 'ttest' as the default statistical_test for the current implem of CFI, which doesn't use CV.
• We can keep the implementation of nb-ttest. I fixed the hardcoded test_frac. 'nb-ttest' will be the default statistical_test for LOCO_CV, CFI_CV ... in Cross Validation #449

Let me know if that's good for you. Sorry for the confusion.

[jpaillard]

If my last two comments are OK for you, @bthirion, this is ready to merge.
It's otherwise blocking #449

[lionelkusch]

@jpaillard I let you deal with this PR.

[jpaillard]

I take the opportunity to rename the function loco --> loco_analysis as discussed in #499

[bthirion]

Very minor stuff pending. Thx !

[jpaillard]

Sorry again for the confusion regarding the default test. I explained the choice a few comments above.

Regarding the statistical test, the current issue is that the cross-validation scheme has not yet been implemented. Currently, the statistical test is performed using a single train/test split. In that case, when considering the loss values for each individual sample of the test set, they can be considered as independent and we could use ttest instead of nb-ttest as the default.

The NB-t-test actually needs to be the default when CV is used, and losses over test sets cannot be considered as independent.
...

[bthirion]

Sorry again for the confusion regarding the default test. I explained the choice a few comments above.

Regarding the statistical test, the current issue is that the cross-validation scheme has not yet been implemented. Currently, the statistical test is performed using a single train/test split. In that case, when considering the loss values for each individual sample of the test set, they can be considered as independent and we could use ttest instead of nb-ttest as the default.
The NB-t-test actually needs to be the default when CV is used, and losses over test sets cannot be considered as independent.
...

OK, makes sense.

[bthirion]

I think it's OK for merging.