Julien Jerphanion

Results 304 comments of Julien Jerphanion

> I think the original intention of the ASV benchmark suite was to continuously monitor user facing classes with a public API that remains stable across scikit-learn versions so as...

> Thanks for the updated PR. I assume that merging with main is needed before starting to review this. You're welcome! I did not know if this PR was submitted...

I've adapted the description with updated benchmarks script and results. It looks like the implementations scales well on the `(n_samples_train, n_samples_test) = (int(1e7), 1000)` case. On `main`, the execution times...

> We are on the path to make everything use Tempita :) It looks like yes. IMO, even if it's suboptimal, restrictive and hard to maintain on the long run,...

Thanks @ogrisel and @thomasjpfan for the reviews!

Oops, renaming branches creates a new branch, delete the previous one and close its associated PRs. Let's restore it and reopen this PR.

The benchmarks shows that the results are significantly slower in this PR, especially on small datasets. E.g. Benchmark run on `metric=manhattan` to use `_sparse_manhattan` on `main`, a simple and fast...

I've opened https://github.com/jjerphan/scikit-learn/pull/15/ to test this second solution, namely: > Keep the array of n_features elements for indices but change the signature of `DistanceMetric.{dist_csr,rdist_csr}` to take pointers and perform shifting...

Before proceeding with further benchmarks, I would wait for https://github.com/scikit-learn/scikit-learn/pull/23865 to be merged, as it will come with adaptations for this PR.

The alternative proposed in https://github.com/jjerphan/scikit-learn/pull/15/ performs better, see [this comment](https://github.com/jjerphan/scikit-learn/pull/15/#issuecomment-1209149658). I would: - merge https://github.com/scikit-learn/scikit-learn/pull/24120 - merge https://github.com/scikit-learn/scikit-learn/pull/23865 - merge https://github.com/scikit-learn/scikit-learn/pull/24043 - merge https://github.com/jjerphan/scikit-learn/pull/15/ in this PR branch - merge...