Comparing python objects such as list and tuple with mlx.core.array (#866)

[amirhossein-razlighi]

Proposed changes

This pull request is related to issue #866 . When trying to compare python list or tuple with an mlx.core.array, mlx throws exception. Right now, regarding the comments in the related issue, mlx returns False (following the way it is done in PyTorch). An example is as follows:

import mlx.core as mx

a = mx.array([1, 2, 3])
a == [1, 2, 3] # False
a == mx.array([1,2,3]) # True
a != [1, 2, 3] # True
a == (1, 2) # False

Checklist

Put an x in the boxes that apply.

• I have read the CONTRIBUTING document
• I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
• I have added tests that prove my fix is effective or that my feature works
• I have updated the necessary documentation (if needed)

[awni]

Thanks for getting this started. Left some comments inline, please address.

[awni]

You had a different version earlier which just pulled out an object if it existed. So if it's not an nb::oject then it's ok (int, bool, float, complex) and if it is then check if its an array or has the attribute? Maybe we should do that instead?

[amirhossein-razlighi]

It's the same line that is present here:

else if (auto pv = std::get_if<nb::object>(&v); pv) {
    return nb::hasattr(*pv, "__mlx_array__") || nb::isinstance<array>(*pv);
  }

The function that we had earlier (isMlxCoreArray) checked this line. I think right now it's better and we may use this utility function further in more operations (where we need to check if the operands are valid and can be converted to mlx.core.array).

[awni]

Ok just thinking about it from an efficiency standpoint (if we wanted to use this in a lot of ops we'd need it to be very fast since it's pure overhead). Can you not do?

if (auto pv = std::get_if<nb::object>(&v); pv) {
    return nb::isinstance<array>(*pv) || nb::hasattr(*pv, "__mlx_array__");
} else {
    return true;
}

Also it might be minor but I would swap the hasattr and the isinstance since the former will be much much more common (and also probably a lot less expensive).

[amirhossein-razlighi]

I swapped the hasattr and isinstance. Also, why should we add else { return true; }? Because if it's not each of the options mentioned above (bool/int/float/complex/array) it is not convertible to array and thus, we should simply return False.

[amirhossein-razlighi]

please check the latest commit and let me know about what you think should be changed

[amirhossein-razlighi]

Also, I think we need a better name for this function. Because, instances like list and tuple are actually convertible to mlx.core.array. They are not comparable to arrays. Don't you agree? This name might be a little misleading in further developments

[awni]

Looks really nice, thank you! I left a couple comments. Could you please take a look and then we can run tests and merge?