Hi. I was wondering, is there any reason, why all np.void (meaning dtype.kind == 'V') arrays are also getting converted to float64? Afaik, np.void is only used for structured dtypes.

So with the proposed changes

named_uint64_dtype = np.dtype([('named_value', np.uint64, (1,))])
named_uint64_array = np.full((1, 1), np.iinfo(np.uint64).max, dtype=named_uint64_dtype)
print(named_uint64_array) # [([18446744073709551615],)]
checked = sklearn.utils.validation.check_array(named_uint64_array)
print(checked) # [[1.8447e+19]]

Notice, that the checked array is silently converted to np.float64, which actually changes the value slightly, since the uint64 value range can't be exactly represented by 64 bit floats.

Also, structured dtypes with more than 1 element now will just break, when trying to convert to np.float64:

custom_complex_dtype = np.dtype([('real', np.float, (1,)), ('imag', np.float, (1,))])
custom_complex_array = np.zeros((1, 1), dtype=custom_complex_dtype)
print(custom_complex_array) # [[([0.], [0.])]]
sklearn.utils.validation.check_array(custom_complex_array)
# TypeError: Cannot cast array data from dtype([('real', '<f8', (1,)), ('imag', '<f8', (1,))]) to dtype('float64') according to the rule 'unsafe'

I am not sure, whether the above is a desirable behaviour. It seems to me, that the "convert strings/bytes to floats" issue is covered by just converting dtype.kind in 'OUS' dtypes?

@ogrisel well, I would have preferred to keep the current behavior of passing the array unchanged for structured dtypes, since there are currently no known (afaik) issues with void typed ndarrays. Somebody (ahem, me) might be passing structured dtype arrays through the scikit machinery, relying on the fact that structured dtypes are preserved.

Although, I'd understand, if you still chose to reject void arrays, since this is definitely not a documented feature.

@ogrisel So you are proposing to raise a new warning and wait for 2 more versions before raising the error?

Would be fine with me. @thomasjpfan does it look OK with you?

@ogrisel well, I would have preferred to keep the current behavior of passing the array unchanged for structured dtypes, since there are currently no known (afaik) issues with void typed ndarrays. Somebody (ahem, me) might be passing structured dtype arrays through the scikit machinery, relying on the fact that structured dtypes are preserved.

That will probably crash with a lower level error message on many scikit-learn estimators, for instance those written in Cython, right?

What is your use case @ruro? Which sklearn estimators do you feed structured dtypes?

Although, I'd understand, if you still chose to reject void arrays, since this is definitely not a documented feature.

Yes sorry I don't thing we should support untested features that lack a meaning usecase.

@ogrisel So you are proposing to raise a new warning and wait for 2 more versions before raising the error?

I was proposing to start the deprecation for any string / bytes arrays in 0.24 (schedule for removal in 0.26).

What is your use case @ruro? Which sklearn estimators do you feed structured dtypes?

Rather than feeding structured dtypes to built-in estimators, the main use case IMO, is when sklearn acts as a middleman between 2 pieces of user code. For example, some estimators might allow the user to provide their own custom metrics, loss functions etc. In these cases sklearn often tends to compulsively cast everything to float, which is kind of annoying. You also have to pack features of different types (categorical, ordinal, boolean, real valued, textual etc) into composite dtypes, if you don't want to end up with floats inside your losses/metrics.

As I mentioned, my particular use case is 100% in the "definitely-not-supported-messing-with-sklearn-internals" category.
I use the following hack

vbrute = sklearn.neighbors.VALID_METRICS['brute']
vmetrics = sklearn.metrics.pairwise._VALID_METRICS
pairwise = sklearn.metrics.pairwise.PAIRWISE_DISTANCE_FUNCTIONS

def install_metric(f):
    name = f.__name__
    if name not in vbrute:
        vbrute.append(name)
    if name not in vmetrics:
        vmetrics.append(name)

    pairwise[name] = f
    return f

Which allows me to register my own custom vectorized metrics:

@install_metric
def custom_metric(X, Z, **kwargs):
    """
    params:
        X: ndarray of shape {x_samples, features}
        Z: ndarray of shape {z_samples, features}
    returns: ndarray of shape {x_samples, z_samples}
    """
    pass

And then use them in any sklearn algo, which accepts a metric:

nn = sklearn.neighbors.NearestNeighbors(
    n_neighbors=3, algorithm='brute',
    metric='custom_metric', metric_params={'kwargs': 'here'}
)

I know, that you can pass custom metric functions as pyfunc, but this evaluates the function for each sample, which is insanely slow and unconditionally casts everything to float, which is really annoying.

There are probably similar (in spirit) use cases which don't rely on messing with sklearn internals, but I don't know any.

ping @adrinjalali :)

I wouldn't mind ignoring the void issue, I guess in almost all cases, if the user passes that kinda data, they know what they're doing.

On silently converting strings to numeric, I'd side on always warn, since this PR can make things a bit inconsistent. For instance, if a dataframe/array has some string columns, and in some folds we can convert things to numeric and in some folds we can't, then the two sets of folds are gonna show a very different behavior. I rather always warn in these cases.

Updated PR with a warning when a conversation takes place.

@ogrisel If I follow the conversation and the code correctly, your requested changes are addressed and this PR is ready for merge, right?

Thank you for finishing this up @ogrisel !