[MRG] Deprecate WMinkowskiDistance & make MinkowskiDistance accept weights

[yarkhinephyo]

Reference Issues/PRs

Resolves #21765

What does this implement/fix? Explain your changes.

• Add FutureWarning to _dist_metrics.WMinkowskiDistance
• Modify _dist_metrics.MinkowskiDistance to accept optional weights w
• Validate the values in w
• Update tests and documentation accordingly

Any other comments?

Nil

[ogrisel]

Thank you very much for the follow-up PR. This is really appreciated. Please find some suggestions/comments to address below:

[yarkhinephyo]

Thank you so much for the detailed suggestions @ogrisel! I have incorporated the changes.

[ogrisel]

Thanks, the updated PR looks good to me!

Can you please run a quick interactive benchmark using ipython or jupyter and the %timeit magic command to check that this does not cause a significant performance regression compared to main when computing unweighted pairwise Minkowski distances between two random arrays in a 10d dimensional space?

The extra if has_w: check is probably degrading the performance by up to 30% or so in very low dimensional space (e.g. 2d or 3d) but so be it.

[yarkhinephyo]

Main branch's vs current branch's unweighted Minkowski -

from sklearn.utils import check_random_state
from sklearn.metrics import DistanceMetric

rng = check_random_state(0)
d = 10
X1 = rng.random_sample((100, d)).astype("float64", copy=False)
X2 = rng.random_sample((100, d)).astype("float64", copy=False)

dm = DistanceMetric.get_metric("minkowski", p=3)

%%timeit
dm.pairwise(X1, X2)

# Main branch, d = 2
1.2 ms ± 37.2 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
# Current branch, d = 2
1.31 ms ± 29.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

# Main branch, d = 10
3.79 ms ± 154 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
# Current branch, d = 10
3.74 ms ± 84.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

[ogrisel]

Great, so no performance problems. Thanks for benchmarking.

[ogrisel]

The sphinx warnings from https://165839-843222-gh.circle-artifacts.com/0/doc/_changed.html seems unrelated to this PR. Let me push a merge commit to check if they have been concurrently resolved in main.

[jjerphan]

Thank you for this contribution, @yarkhinephyo.

Here are a few comments.

[jjerphan]

LGTM after formatting with black.

[jjerphan]

The errors on the CI here look unrelated to your PR.

Can you re-trigger the CI by adding an empty commit, @yarkhinephyo? This can be done by:

git commit --allow-empty -m "Just to trigger the CI"

Edit: I just did it to move forward.

[jjerphan]

It indeed just was a random failure on the CI.