Why not let pairwise_distances give this warning? The concern here is also that we issue this warning before we actually do the conversion.

You're right, I looked into it and it looks like @IgnacioJPickering just moved the the existing logic inside the helper function.

The old code in pairwise_distances had a manual block that issued this exact warning:

 dtype = bool if metric in PAIRWISE_BOOLEAN_FUNCTIONS else "infer_float"

        if dtype is bool and (X.dtype != bool or (Y is not None and Y.dtype != bool)):
            msg = "Data was converted to boolean for metric %s" % metric
            warnings.warn(msg, DataConversionWarning)

        X, Y = check_pairwise_arrays(
            X, Y, dtype=dtype, ensure_all_finite=ensure_all_finite
        )

        # precompute data-derived metric params
        params = _precompute_metric_params(X, Y, metric=metric, **kwds)
        kwds.update(**params)

        if effective_n_jobs(n_jobs) == 1 and X is Y:
            return distance.squareform(distance.pdist(X, metric=metric, **kwds))
        func = partial(distance.cdist, metric=metric, **kwds)

it is now moved into the helper functions and used as such:

     X, Y, dtype = _find_dtype_for_check_pairwise_arrays(X, Y, metric)
       X, Y = check_pairwise_arrays(
           X, Y, dtype=dtype, ensure_all_finite=ensure_all_finite
       )

       # precompute data-derived metric params
       params = _precompute_metric_params(X, Y, metric=metric, **kwds)
       kwds.update(**params)

       if effective_n_jobs(n_jobs) == 1 and X is Y:
           return distance.squareform(distance.pdist(X, metric=metric, **kwds))
       func = partial(distance.cdist, metric=metric, **kwds)

   return _parallel_pairwise(X, Y, func, n_jobs, **kwds)

we are actually issuing the waring after the conversion as it was before the refactor.

@lucyleeow I understand the concern, its not great that the warning is for what a different function does (I believe what @pushkar-hue means is that this is the way it was done before in the code too, but I see its not optimal)

The reason we can't delegate the warning to pairwise_distances is because that function may not get called if ArgKMin is usable for the metric.

Do you think it would be better to write a wrapper _check_pairwise_arrays_for_metric(...)? This would find the dtype, raise the warning and also call check_pairwise_arrays which is what ultimately does the conversion.

@lucyleeow I went ahead and did this, since I thought it was cleaner. and also clarified the comment and made it a bit more precise. Hopefully things are much more clear now.

The reason we can't delegate the warning to pairwise_distances is because that function may not get called if ArgKMin is usable for the metric.

Aren't bool metrics specifically excluded from ArgKMin?

The issue is that check_pairwise_arrays has to be called before the ArgKmin.is_usable_for(...)

Why is this a problem?

I don't understand https://github.com/scikit-learn/scikit-learn/pull/32511/files#r2485450646: check_pairwise_arrays would no longer be called at all for PAIRWISE_BOOLEAN metrics: so error messages about inconsistent shapes would not be raised when passing invalid inputs for boolean metrics.

@ogrisel check_pairwise_arrays would still be called, since for boolean metrics the function would delegate to pairwise_distances_chunked, which itself delegates to pairwise_distances, which calls check_pairwise_arrays. I recognize it is a bit confusing though, but I think the logic checks out.

@lucyleeow It is not a problem, but we must first filter for boolean arrays. Calling check_pairwise_arrays in these functions without first filtering for boolean metrics is the bug that is currently in main.

By default, if check_pairwise_arrays is called with infer_float it converts the arrays to float unconditionally, with no warning, which is not necessary since the arrays are bool to begin with.

Afterwards a second check in pairwise_distances checks the dtypes of the arrays, converts the arrays back to bool, and raises a conversion warning. This means there are 2 casts and 1 warning where there should have been none.

If I filter for PAIRWISE_BOOLEAN first then I can call check_pairwise_arrays only in the case that ArgKmin may be called, and delegate the rest of the checks to pairwise_distances_chunked.

@lucyleeow @ogrisel
From the comments in the PR I believe this, together with a comment specifying why check_pairwise_arrays is being called early in the case that ArgKmin may be called, is preferable, since it seems to me the code is still hard to understand.

Ok, I've modified the code so that the checks are delegated to pairwise_distances_chunked I believe this should get rid of the confusion.

The TypeError is raised only in the case where sparse arrays are forwarded to pairwise_distances, which is a single place in the code, the same place where the warning is raised, and the cast is performed.

This only required an if check for the check_pairwise_arrays call, which is only performed if the metric is not a PAIRWISE_BOOLEAN, so we avoid casting it to bool like what is currently in main. I added a comment for extra clarity.

In the end I think my initial fix was overly complicated, this gets rid of the issue and has minimal modifications.