Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

np.floor_divide(?, ml_dtypes.???(0.0)) return NaN but np.float16 returns Inf. #170

Closed
apivovarov opened this issue Aug 22, 2024 · 3 comments · Fixed by #172
Closed

np.floor_divide(?, ml_dtypes.???(0.0)) return NaN but np.float16 returns Inf. #170

apivovarov opened this issue Aug 22, 2024 · 3 comments · Fixed by #172
Assignees
Labels
bug Something isn't working

Comments

@apivovarov
Copy link
Contributor

If the second argument of np.floor_divide is 0.0, it returns different results depending on the type: np.dtypes returns Inf, while ml_dtypes returns NaN

Related test: ml_dtypes/tests/custom_float_test.py testBinaryUfunc. The test fails if y has 0.0 elements.

Manual run to confirm the difference in behavior

>>> np.floor_divide(ml_dtypes.float8_e4m3(1.0), ml_dtypes.float8_e4m3(0.0))
nan
>>> np.floor_divide(ml_dtypes.float8_e5m2(1.0), ml_dtypes.float8_e5m2(0.0))
nan
>>> np.floor_divide(ml_dtypes.bfloat16(1.0), ml_dtypes.bfloat16(0.0))
nan
>>> np.floor_divide(np.float16(1.0), np.float16(0.0))
np.float16(inf)
@jakevdp
Copy link
Collaborator

jakevdp commented Aug 22, 2024

Hi - thanks for the report! It looks like that output comes from this line:

return {nan, nan};

We probably need to update this to condition the returned value on the value of a.

I think it should return {-inf, nan} if a < 0, {nan, nan} if a == 0, and {inf, nan} if a > 0.

@jakevdp jakevdp added the bug Something isn't working label Aug 22, 2024
@jakevdp jakevdp self-assigned this Aug 22, 2024
@jakevdp
Copy link
Collaborator

jakevdp commented Aug 23, 2024

Thanks again for the report! We'll try to cut a new release soon with this fix. Let us know if you run into other issues

@apivovarov
Copy link
Contributor Author

apivovarov commented Aug 23, 2024

Thank you! It would be great if we could also include float8_e3m4 in the new release.

PR-171 which adds float8_e3m4 is a clone of the previously merged PR-161 that added float8_e4m3.

One of the float8_e3m4 tests exposed an issue with floor_divide when random data was generated for float8_e3m4 type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants