thanks for the review @agramfort

The LocalOutlierFactor benchmark is now done in an outlier detection context.

Concerning LOF, standardizing the datasets (using StandardScaler or RobustScaler) does not increase the performance in terms of ROC curve and AUC.

LGTM +1 for MRG

Indeed... thanks @ogrisel. We also have this warning on master. That's because there are only 3 outliers out of 9571 samples in the smtp dataset when percent10=True. Therefore there is a high chance that there is no outlier in the test set (when the model is trained on a dataset that can contain outliers). Of course this depends on the random seed set in np.random.seedat the beginning of the benchmark.

One solution would be to remove the smtp dataset from the benchmark. If percent10=False the problem is less likely to happen but may still happen depending on the random seed.

Note that we don't have this problem for the LOF benchmark anymore as we train and test on the whole dataset. We wouldn't have the error/warning if the isolation forest benchmark were run similarly as the LOF benchmark. We wouldn't have this problem either if the isolation benchmark were run in a novelty detection context (train the model on a subsample of normal instances only and test it on all the other instances)

Interestingly, for the same seed in np.random.seed, if the benchmark starts with the 'smtp' dataset, the problem disappears (because the state of the RNG is not the same as when the benchmark starts with 'http'). For this reason it might be better to set the seed via the random_state parameter of the fetch_kddcup_99, fetch_covtype and shuffle functions rather than setting it with np.random.seed at the beginning of the dataset. The warning/error should then appear (or don't appear) whatever the order of the datasets.

I clarified the randomness in both benchmarks and thus made the warning deterministic for the Isolation Forest benchmark. I also added a comment about it in the docstring.

Performance is not convincing for LOF, even in an outlier detection context. I did not review the PR, but did you try playing with the threshold/offset parameter? And is there any reason why you changed percebt10 to True? (It is not the cases for other AD benchmarks is it?)

percent10 was set to True for IsolationForest. I also set it to True for LOF. Performance is not great either if percent10 is set to False for LOF. LOF seems to be performing well on the considered datasets in a novelty detection context, however LOF is not meant to be used in this context. To show that LOF can perform well in a outlier detection context would imply to consider new datasets (see @ngoix extensive benchmark)

Changing the threshold parameter will not affect the performance as we assess the whole scoring function with a ROC curve.

You're right my mistake!

But IMO benchmarks are not examples, they are made to compare performance. To do this, we need to choose a commun setting for all AD algo (btw it is one of the reason why we chose to implement a predict and decision function methods for LOF, even if private).
An interesting change on the benchmarks would be to create a file per setting (outlier/anomaly/novelty detection), each file including all AD algo. But this PR is probably not the place to do it.

All in all, I think we should stick to the fix of #8638 in this PR.

btw @albertcthomas, have you tried to benchmark with and without shuffling data? It looks like data are not shuffled anymore with your changes

It looks like they are shuffled when we change the SEED. For the 'http' dataset, with SEED = 1,

X[0] = [-2.3025850929940455, 5.313698468586339, 5.59508305773286]

with SEED = 12,

X[0] = [-2.3025850929940455, 5.656341400056065, 6.319148277696116]

Results of the whole benchmark with SEED = 1 for the shuffling and random_state=1 for IsolationForest,

with SEED = 12 for the shuffling and random_state=1 for IsolationForest

We can observe small differences in terms of AUC coming from the fact that the datasets are shuffled.

Yes I meant for LOF, you removed the shuffle argument which is set to false by default I believe...

The LOF benchmark is not random anymore because the model is trained and tested on the same dataset, so there is no need to shuffle the datasets. Setting shuffle to True does not change the results. I made a comment about this in the docstring of the benchmark.

Ok I get it now !
So WDYT about my previous comments before this big "by the way" :) ?

About making one file per setting (outlier/anomaly/novelty), each comparing all outlier detection estimators? I'm +1 for this in a new PR.

Then you don't need any change in bench_lof.py...

The decision to make modifications to the LOF benchmark came after this discussion.

We could use _decision_function in the LOF benchmark as we also use _decision_function for LOF in the outlier detection examples (see here). But IMO this could cause confusion and give a wrong message to the users, as in issue #9874 maybe...
@agramfort what do you think?

Codecov Report

Merging #9798 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9798   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files         336      336           
  Lines       62493    62493           
=======================================
  Hits        60098    60098           
  Misses       2395     2395

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 cf174af...3e06c31. Read the comment docs.

@albertcthomas I understand your point, but I think that unlike examples, benchmarks are not meant to guide the user but to compare algorithms performance.

Ok @agramfort but then we also have to remove LOF from the outlier detection example as it uses _decision_fucntion.
Beside, we can't make the two different benchmarks we discussed above (outlier / anomaly detection), each benchmark containing all AD algo.

just ran the bench locally. Worked great.

good to go @ogrisel or @lesteve ?

I had a quick IRL conversation with @albertcthomas and I think we should convert those 2 benchmark scripts into a single example script with a plt.subplot grid, one subplot per dataset with both LOF and IF ROC curves.

Also I think having to convert the categorical labels as strings to get LabelBinarizer to behave correctly is a bug. It should work with an array of object labels which is a more memory efficient representation. But this should probably be tackled in a separate PR.

Actually, we should just use the categorical encoder once it's ready.

Let's merge this as this is arlready a good fix in itself and the conversion into an example can be done in another PR.