Keep double precision for python floats in float64 operations - #4173
Keep double precision for python floats in float64 operations#4173nileshpatil6 wants to merge 2 commits into
Conversation
|
hey! I post you here three cases that current tests misses, see if some it's worth to add:
separately, |
a754bfb to
21bbf39
Compare
|
Thanks, all four are real. Pushed 21bbf39 covering them. The overflow case is the best argument for the fix, so I led the tests with it.
Added the dtype pinning too: float16, bfloat16 and float32 arrays still keep their own dtype against a python float, and an int32 array still gives float32. That's the weak-scalar rule the fix has to leave alone, so it's worth having a test hold it in place.
I also went through the other test_double, test_array, test_ops, test_autograd, test_blas and test_einsum all pass on CPU. |
Review follow-ups applied locally (verification-driven)A :
|
|
Follow-up commit pushed as a second commit (no amend/force-push), targeting your branch:
I do not have permission to edit this PR description. Could you add this paragraph so maintainers see the
Verified before push:
|
|
Three corrections to my comment above, after checking against the source:
|
|
@nileshpatil6 could you add this paragraph to the #4173 description? I don't have edit rights on the upstream PR. Behaviour note: since pad now resolves Follow-up is now tests-only (47f742b): nileshpatil6#1 |
|
@AxelNoun thanks for the thorough pass, and especially for catching the int overflow side of the pad change. You're right that it deserved to be called out as a behavior change and not left implicit; I've added the paragraph to the description. Verified on my build too: int8 with 300 and uint8 with -1 both raise the same "would result in overflow" error that mx.full already produced for those pairs. Also appreciate you walking back the float64-branch simplification yourself after checking the ARM float16 path. Narrowing through float32 vs a single double-to-half rounding is exactly the kind of thing that would have surfaced as a mystery test failure on Apple silicon later, so it's good that stays as is. On the follow-up branch: since zcbenz has already approved this and it's waiting on CI, I'd rather not add commits to it at this point. Your additions are tests-only now, so nothing in this PR depends on them; I think the cleanest path is to open them upstream as your own PR once this lands. They're your tests and you should carry the authorship. Happy to comment in support when you do. |
|
Can you fix the lint error? |
Works for me, an approved PR sitting on CI isn't the place for extra Thanks for adding the behaviour paragraph and verifying the overflow |
Fixes #4160, fixes #4159.
Both issues come down to the same line in
to_array(python/src/utils.cpp): a python float is converted withnb::cast<float>before the target dtype is applied, so the value is rounded to float32 precision and then stored in a float64 array. That's whymx.full((3,), 0.37, dtype=mx.float64)holds0.3700000047683716, and whyfloat64_array * 0.1computes withfloat32(0.1)while reporting a float64 result.to_arraysroutes weak python scalars through this same function with the other operand's dtype, which is how the arithmetic case ends up here too.The fix casts to double when the resolved output dtype is float64, and leaves every other path exactly as it was. This mirrors what
create_arrayin convert.cpp already does formx.array(v, dtype=mx.float64), which is why the workaround in #4160 produced the right value. Weak promotion is untouched: a python float still doesn't promote a float32 array, it just stops losing digits when the expression is already float64.Added a regression test to
test_double.pycoveringmx.fullwith an explicit float64 dtype, float64-array-times-python-float for a few non-float32-exact literals (0.1, 1e-4, pi), and a check that float32 stays the default and isn't promoted. The new test fails on main and passes with this change.test_double,test_array,test_ops,test_autograd, andtest_blasall pass on CPU (Linux, no Metal). clang-format and black are clean.Behavior note on the pad change (thanks @AxelNoun for flagging it): since
padnow resolvesconstant_valuesin the input's dtype, an out-of-range python int raises instead of wrapping silently.mx.pad(mx.array([1], mx.int8), 1, constant_values=300)used to produce 44, andconstant_values=-1on a uint8 array produced 255; both now raise "Converting ... would result in overflow", which is exactly whatmx.full(shape, 300, dtype=mx.int8)already did for the same value and dtype.