ml-explore / mlx

MLX: An array framework for Apple silicon
https://ml-explore.github.io/mlx/
MIT License
17.01k stars 984 forks source link

[Feature] arctan2 #1065

Closed yrahul3910 closed 6 months ago

yrahul3910 commented 6 months ago

I want to eventually contribute to this Keras issue, and since this would be my first contribution, I want to start small and implement arctan2. If nobody is currently working on this, I would like to contribute this feature. Specifically, my implementation plan, based on looking at arctan, is as follows:

Please let me know if I can proceed.

awni commented 6 months ago

Your description is right modulo the fact that it is a binary op not a unary op.

I would look at another binary op like Add as an example. Most of the implementation should be boiler plate copying of another binary primitive. The core change will be in the op struct which gets passed to the CPU / GPU binary op kernel.

awni commented 6 months ago

And yes please add this, that would be great!

yrahul3910 commented 6 months ago

I think I did it :) When running make, I get warnings about the use of binary(...) (but it successfully builds and tests pass). This is because the binary function also tests bool, so std::atan2 warns about precision loss. I'm not entirely sure what to do there--maybe for future binary (but not bool) ops we have a different function called binary_fp, like we have for unary ops?