Skip to content

Fix GPU ABS range - #4345

Open
JasonHonKL wants to merge 2 commits into
ml-explore:mainfrom
JasonHonKL:issue/4344
Open

Fix GPU ABS range#4345
JasonHonKL wants to merge 2 commits into
ml-explore:mainfrom
JasonHonKL:issue/4344

Conversation

@JasonHonKL

@JasonHonKL JasonHonKL commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

Fix #4344

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

return bfloat16_t(x * (metal::log(xp1) / (xp1 - 1.0f)));
}

inline float complex_abs(float x, float y) {

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.

Since this is only used once I think we should just put it in operator()(complex64_t x)

@JasonHonKL

Copy link
Copy Markdown
Contributor Author

Hi @zcbenz gonna resolve this weekend having a trip now sorry !

@eyupcanakman

Copy link
Copy Markdown
Contributor

Two more sites carry the same sqrt(re*re + im*im).
Power builds x_ln_r from it in binary_ops.h, and Sign divides by it in unary_ops.h.
Both still come back nan with your patch applied.

I put your diff on a Metal build here (M5 Pro) and ran the two values from the issue through sign and power:

gpu, complex64            #4345 alone   + the two edits              cpu
sign   (2.5e-20,-3e-20)   (nan, nan)    (0.640184, -0.768221)        (0.640184, -0.768221)
z**0.5 (2.5e-20,-3e-20)   (nan, nan)    (1.78957e-10, -8.3819e-11)   (1.78957e-10, -8.3819e-11)
z**2   (1e20, 1e20)       (nan, nan)    (-inf, inf)                  (-inf, inf)
z**2   (2.5e-20,-3e-20)   (nan, nan)    (-0, -0)                     (-2.75001e-40, -1.5e-39)

The last row stays off because the true result is subnormal and Metal flushes it, which is separate from this.

Power takes the one-line swap you already wrote, and the 0.5 falls out since complex_abs gives the magnitude rather than its square.

Sign needs a little more.
x / (complex64_t)complex_abs(x.real, x.imag) still returns nan, since operator/ in complex.h squares its own denominator, so the magnitude goes out of float32 range there instead.
Two float divisions by complex_abs(x.real, x.imag) get around it.

Either way the helper ends up with three callers across two headers, which is the argument for leaving it in utils.h.

@zcbenz zcbenz added the await response This pull request is waiting for response from the author. label Aug 25, 2026
@zcbenz

zcbenz commented Aug 25, 2026

Copy link
Copy Markdown
Member

We should add a hypot utility if we want to reuse the code:
https://github.com/pytorch/pytorch/blob/a82aae9d4a7827849ce50f31c4c7ee8f278d05f5/c10/metal/utils.h#L554

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

Labels

await response This pull request is waiting for response from the author. low priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] mx.abs / mx.sqrt / mx.log on complex64 are silently wrong on GPU across a third of float32's range

3 participants