Closed balancap closed 1 day ago
This looks good to me. There's a small failure in the test suite: please fix?
(Sorry about the slow review.)
Thanks for looking at it, I'll check the failing tests in the next few days. In the meantime, do you have an opinion on the naming? Should we stick to the name in the PR?
@hawkinsp Apologies it took me a bit longer than expected. The CI issue should be fixed now (was just a trivial skipTest
issue with Python 3.12). I have rebased the branch as well on the latest main
This LGTM, but I'd appreciate @sergey-kozub's review, since he's in the process of adding the (complementary) MX data types in https://github.com/jax-ml/ml_dtypes/pull/181
There's a test failure on mac os (different integer overflow behavior?)
Overall looks fine to me.
This PR touches many files also affected by #181, so I'd like to upstream that one first. I'll do the rebase to fix the merge failures.
@sergey-kozub @hawkinsp Removing 0
fixes the MacOS test run, but I am still investigating a crash on the Windows build (stack overflow).
Ideally we'd like to make a new ml_dtypes
release in the next few days, and it'd be great to get both of the MX dtype PRs in.
@hawkinsp Just pushed a fix commit for MacOS and Windows. Hopefully should be the last one if @sergey-kozub is good with the PR as it is.
I'm trying to merge https://github.com/jax-ml/ml_dtypes/pull/181 first, which will probably create a merge conflict and you'll need to rebase on top of that PR. Hopefully it will be in soon.
Ok, #181 is merged.
Would you: a) squash your commits, please, and b) rebase on top of head?
Oh, it looks like you have a test failure on Python 3.13t. I doubt the "free-threading" part is important, but we are trying to make a Python 3.13 release.
I'll rebase & squash commits. On the Python 3.13 failure, we should probably skip this test as float8_e8m0
does not have a zero (or negative zero!). Just surprising it triggers on error only on Python 3.13
@hawkinsp I should be hopefully the last round of rebase & fix.
I added a skip on the test testHashZero
, as E8M0
does not have a zero representation. I can dig a bit more into why the test is failing on Python 3.13, but I don't think it should block the PR merge.
Note: there is a new failure on Python 3.9 introduced by the previous MX float4/float6 PR. I am looking into it.
The trick cls._finfo_cache[dtype] = init.__func__()
seems to do it for Python 3.9. CI all green on my fork, finally!
What went wrong without init.__func__()
? @sergey-kozub had that in his PR, but i had to revert it because it hit a type checker error. But I'm not sure what problem it's solving.
Python 3.9 does not seem to support the direct call to staticmethod
(getting this error without __func__()
: https://stackoverflow.com/questions/41921255/staticmethod-object-is-not-callable)
The merge commit on the main branch has this issue: https://github.com/jax-ml/ml_dtypes/actions/runs/10833234151/job/30059511975
Adding the OCP MX scale format
E8M0
, which has the following properties:The
E8M0
format is used in all MX block format definitions for scale representation (see https://www.opencompute.org/documents/ocp-microscaling-formats-mx-v1-0-spec-final-pdf and issue #116).ml_dtypes
float8_base
C++ class is extended to support floating point formats which are unsigned and with no zero (i.e. additionalkIsSigned
andkHasZero
Traits properties).Base on these traits,
float8_e8m0_fnu
has been implemented using the existing functionalities (convert, unary/binary ops, ...). Float8 Python unit tests have been extended to be able to cover unsigned floating point formats.Points to discuss on the PR:
float8_e8m0_fnu
. Maybeufloat8_e8m0_fn
could be better?E8M0
. Is there a cleaner way?