Skip to content

Keep double precision for python floats in float64 operations - #4173

Open
nileshpatil6 wants to merge 2 commits into
ml-explore:mainfrom
nileshpatil6:fix-float64-python-scalars
Open

Keep double precision for python floats in float64 operations#4173
nileshpatil6 wants to merge 2 commits into
ml-explore:mainfrom
nileshpatil6:fix-float64-python-scalars

Conversation

@nileshpatil6

@nileshpatil6 nileshpatil6 commented Aug 11, 2026

Copy link
Copy Markdown

Fixes #4160, fixes #4159.

Both issues come down to the same line in to_array (python/src/utils.cpp): a python float is converted with nb::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 why mx.full((3,), 0.37, dtype=mx.float64) holds 0.3700000047683716, and why float64_array * 0.1 computes with float32(0.1) while reporting a float64 result. to_arrays routes 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_array in convert.cpp already does for mx.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.py covering mx.full with 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, and test_blas all pass on CPU (Linux, no Metal). clang-format and black are clean.


Behavior note on the pad change (thanks @AxelNoun for flagging it): since pad now resolves constant_values in 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, and constant_values=-1 on a uint8 array produced 255; both now raise "Converting ... would result in overflow", which is exactly what mx.full(shape, 300, dtype=mx.int8) already did for the same value and dtype.

@robertomeroni

Copy link
Copy Markdown
Contributor

hey! I post you here three cases that current tests misses, see if some it's worth to add:

  • overflow: mx.array([1.0], mx.float64) * 1e300 is inf on main and 1e+300 with your patch; * 1e-300 goes 0.0 -> 1e-300
  • mx.maximum, mx.where and mx.clip truncate identically
  • Pinning float16/bfloat16/int32 too

separately, mx.pad's constant_values passes no dtype to to_array (ops.cpp:3456), so it still truncates. Not covered by either issue

@nileshpatil6
nileshpatil6 force-pushed the fix-float64-python-scalars branch from a754bfb to 21bbf39 Compare August 11, 2026 09:24
@nileshpatil6

Copy link
Copy Markdown
Author

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. 1e300 and 1e-300 aren't just losing digits, they're saturating to inf and 0.0 because the value goes through float32 on the way in. Both are now asserted.

maximum, minimum, clip and where all route their scalar operand through to_arrays, so they were already fixed by the change, but nothing pinned that. Added an assertion for each.

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.

pad was a good catch and I ended up including it. It looked risky at first, but pad casts the fill value to the input's dtype anyway, so passing a.dtype() into to_array doesn't change any result dtype, it only stops the value being rounded to float32 first. Verified before and after across int32/float16/bfloat16/float32/float64: dtypes identical, and mx.pad(float64_arr, 1, constant_values=0.1) goes from 0.10000000149011612 to 0.1. There's a test pinning the dtypes so that stays true.

I also went through the other to_array call sites that don't pass a dtype. The rest are unary ops on a single operand, where there's no companion array to inherit from and float32 is the right default, so pad was the only other one affected.

test_double, test_array, test_ops, test_autograd, test_blas and test_einsum all pass on CPU.

@AxelNoun

AxelNoun commented Aug 11, 2026

Copy link
Copy Markdown

Review follow-ups applied locally (verification-driven)

A : pad integer overflow (confirmed)

On main/mlx[cpu], mx.pad(int32, ..., constant_values=2**40) and mx.pad(int8, ..., 300) silently truncate. On this PR they raise ValueError, matching mx.full. Added test_pad_scalar_overflow in test_ops.py.

Suggested PR description addition:

pad now resolves constant_values in the input dtype, so an out-of-range python int errors instead of silently truncating (mx.pad(int8_arr, 1, constant_values=300)), matching mx.full(shape, 300, dtype=mx.int8).

B : simplify float64 branch (applied; values identical)

mx.full limit table for float16/bfloat16/float32/float64 (incl. 1+2**-24) is bit-identical before/after collapsing to always nb::cast<double> + constructor narrow.

C : left-hand scalar (confirmed + tested)

0.1 * a, 0.1 - a, 1e300 / a, mx.maximum(0.1, zero) keep float64 precision; assertions added.

D

  • D.1: mx.full(..., 1e300/1e-300, float64) assertions added.
  • D.2: indexing.cpp already passes src.dtype() to to_array → assertion only for a[0]=0.1.
  • D.3: remaining no-dtype to_array sites are unaries / both-scalar to_arrays fallback / random bounds (default float32; float64 uniform unsupported on this CPU build) / distributed / logical_* (bool result). No further patch in this PR.

E : moved weak promotion + non-float64 pad dtype checks to test_ops.py::test_weak_scalar_promotion.

Checklist pytest: 332 passed, 24 skipped.

@AxelNoun

AxelNoun commented Aug 11, 2026

Copy link
Copy Markdown

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 pad behavior change?

pad now resolves constant_values in the input dtype, so an out-of-range python int raises instead of being silently truncated (mx.pad(int8_arr, 1, constant_values=300)). That matches the behavior mx.full(shape, 300, dtype=mx.int8) already had; a test pins it.

Verified before push:

  • All new asserts fail on main and pass here, including a[0] = 0.1 (raw main value [0.10000000149011612, 2.0]) so D.2 does exercise the to_array float64 fix via the existing src.dtype() indexing path.
  • pytest test_double.py test_ops.py -rs: 166 passed, 0 skipped. The 24 skips in the broader suite are GPU/MPS/Torch/TF only.

@AxelNoun

Copy link
Copy Markdown

Three corrections to my comment above, after checking against the source:

  • "values identical" for the float64-branch simplification only holds on
    x86, where _MLX_Float16 narrows through float regardless. On ARM
    builds float16_t is native and double→half would single-round
    (1 + 2**-11 + 2**-30: 1.0009765625 vs 1.0 through float32), so I've
    dropped that simplification — the follow-up is now tests-only, and
    utils.cpp stays as in 21bbf39.
  • "all new asserts fail on main" holds for the assertions targeting the
    fix; the weak-promotion ones are guards and pass on both sides.
  • The float64 limitation in random.uniform comes from its dtype switch
    in random.cpp ("[uniform] Unsupported type."), not from the CPU build;
    normal already threads dtype through for loc/scale.

@AxelNoun

Copy link
Copy Markdown

@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 constant_values in 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 give 44,
and constant_values=-1 on a uint8 array gave 255; both now raise,
matching what mx.full already does for the same value/dtype pairs.
test_pad_scalar_overflow pins this.

Follow-up is now tests-only (47f742b): nileshpatil6#1

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nileshpatil6

Copy link
Copy Markdown
Author

@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.

@zcbenz

zcbenz commented Aug 12, 2026

Copy link
Copy Markdown
Member

Can you fix the lint error?

@AxelNoun

Copy link
Copy Markdown

@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.

Works for me, an approved PR sitting on CI isn't the place for extra
commits, and nothing here blocks on the tests.
I'll close this one, keep
the branch, and open the tests upstream once #4173 lands, linking back.

Thanks for adding the behaviour paragraph and verifying the overflow
error on your build, and for the offer
I'll ping you on the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants