Gap encoder speedups

[LeoGrin]

Speedup the GapEncoder.

• Implement a better early stopping for the GapEncoder, based on the score. Benchmarked in Benchmark gap encoder early stopping #681. Based on Benchmark Early Stopping for the GapEncoder #663 and Benchmark GapEncoder divergence #593.

  • Use an Exponentially weighted average of the batch score instead of computing the full score, based on sklearn's MinibatchNMF's code.
  • Remove the early stopping logic based on W_change, as it was never reached. Thus remove the parameters tol.
  • Remove the min_iter parameter.

• Speedup each loop by taking profit of the sparsity of the count matrix. More precisely, compute H @ W only when X is non-zero (this is adapted from sklearn's MinibatchNMF). I sped up the W update which was the bottleneck. The H update was already better optimized for sparsity, but maybe it can also be sped-up using _special_sparse_dot (I won't have time to do it in the near future.)

• Benchmark hyperparameters batch_size, max_iter_e_step and max_no_improvements after these changes. They now default to 1024, 1 and 5. Note: the benchmark should be rerun when we deal with id columns (Handle id columns differently #585), because the hp are chosen to mostly speed up the GapEncoder on these columns, as they are currently the bottleneck. max_iter_e_step should also perhaps be increased if we manage to speedup the H update.

Quick benchmark done on my laptop comparing the speed of this implementation to the current main branch, on the columns of traffic_violations (restricted to 100K rows):

[LilianBoulard]

Nice implem, thanks!
On top of the below comments, it also needs a proper test :)

[LeoGrin]

Nice implem, thanks! On top of the below comments, it also needs a proper test :)

Thanks for the review! What kind of test do you have in mind? A test to check that update_multiplicative_w is the same function before and after the change?

[LilianBoulard]

Maybe test that passing None runs the maximum number of iterations, and with an early stopping value, that it doesn't (if possible).

[GaelVaroquaux]

Very nice. I just gave it a quick try:

On main:

In [1]: from skrub import datasets, TableVectorizer
   ...: 

In [2]: data = datasets.fetch_employee_salaries()

In [3]: tab_vec = TableVectorizer()

In [4]: %time X = tab_vec.fit_transform(data.X)
CPU times: user 36.5 s, sys: 2min 9s, total: 2min 46s
Wall time: 22.3 s

On this branch:

CPU times: user 11 s, sys: 30.9 s, total: 41.9 s
Wall time: 6.22 s

6 seconds has a very very different feel than 22 seconds.

and if I print X and just look at it with my eyes, the results seem exactly the same (so the difference is small). Very nice!!

[GaelVaroquaux]

LGTM. Good for merge as far as I am concerned. Thanks!!

[LeoGrin]

Maybe test that passing None runs the maximum number of iterations, and with an early stopping value, that it doesn't (if possible).

I added a test to check that it doesn't break with max_no_improvement=None, but it seems hard to check the number of iteration (we don't have a n_iter_ attribute). Do you have an idea how it could be done simply? Otherwise, are you fine with merging without this test?

[Vincent-Maladiere]

A few comments and this is good to go!

[Vincent-Maladiere]

This will need further investigation in another PR

[Vincent-Maladiere]

Also, why is the parquet file necessary?

[LeoGrin]

Also, why is the parquet file necessary?

It contains the result of the hyperparameters benchmark

[Vincent-Maladiere]

Yes but still why would we need this file? It's usually not recommended to commit data in repository, the point of having benchmark files is also to reproduce this data.

[LeoGrin]

Yeah I'm not sure. We've been doing this for the previous benchmarks: it's pretty small files, and it makes it straightforward for other people to analyse the results. But I see your point, I'm kinda pro pushing the results but I don't have a strong opinion. Le lun. 7 août 2023 à 23:19, Vincent M ***@***.***> a écrit :

…

Yes but still why would we need this file? It's usually not recommended to commit data in repository, the point of having benchmark files is also to reproduce this data. — Reply to this email directly, view it on GitHub <#680 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AK46V2CXHOYE7ZTDPJOIAW3XUFLUBANCNFSM6AAAAAA23Q7GYE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Vincent-Maladiere]

Ok, let's keep this file and move fast. We'll discuss these benchmark files later :)

[Vincent-Maladiere]

Hey @LeoGrin, could you add a test with verbose=True? That would help with the coverage! We'll merge then.

[jovan-stojanovic]

Hey @LeoGrin thanks, I think this is ready to be merged after you merge with main.

[Vincent-Maladiere]

There's an error that seems unrelated to your PR. I fixed it, could you rebase on main to see if this is still there?

[jovan-stojanovic]

Thanks @LeoGrin !! This will be such an improvement ..

[GaelVaroquaux]

The doc building is indeed markedly faster:

Congratulations!

[Vincent-Maladiere]

where do you see this @GaelVaroquaux ?

[GaelVaroquaux]

I go to the history of commit (from the main github page), and I click on the green "tick" for a given commit, then follow the circleci link.