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

BUG: <uint64 dtype is broken for Max> #770

Open
Dhruvanshu-Joshi opened this issue May 14, 2024 · 3 comments
Open

BUG: <uint64 dtype is broken for Max> #770

Dhruvanshu-Joshi opened this issue May 14, 2024 · 3 comments
Labels
bug Something isn't working C-backend

Comments

@Dhruvanshu-Joshi
Copy link
Member

Describe the issue:

The Max op which is a subclass of CAReduce fails for 64 bit unsigned integers. This is also evident in the PR 731 and its test. The exact number where uint64 starts failing for is 9223372036854775.
Error can also be reproduced with the following example:

Reproducable code example:

import pytensor.tensor as pt
import numpy as np

dtype="uint64"
n = pt.vector("n", shape=(None,), dtype=dtype)
test_n = np.array([0, 9223372036854775], dtype)

assert pt.max(n).dtype == dtype
print(pt.max(n).eval({n: test_n})) # 9223372036854776
print(test_n.max()) # 9223372036854775
assert pt.max(n).eval({n: test_n}) == test_n.max()  # Fails, returns 1 larger

Error message:

No response

PyTensor version information:

Python 3.11.8
Pytensor 2.20.0

Context for the issue:

No response

@Dhruvanshu-Joshi Dhruvanshu-Joshi added the bug Something isn't working label May 14, 2024
@aseyboldt
Copy link
Member

Consider me intrigued...
Sounds like somewhere there is a conversion to float64, because that exact number is one bigger than the largest integer a float64 can represent exactly:
np.float64(9223372036854775).astype(np.int64) is our magic 9223372036854776.

@aseyboldt
Copy link
Member

aseyboldt commented May 14, 2024

This seems to have been around for a long time.
The maximum and minimum are implemented as CAReduction (pytensor.scalar.basic.ScalarMaximum). There seem to be two problems with this implementation: Both scalar Ops specify inf and -inf as identity element, but if we are working with integer types, those do not even have an identity.
But the direct source of the bug seems to be this generated C-Code:

        return f"{z} = (({y})>({x})? ({y}): " f'(({x})>=({y})? ({x}): nan("")));'

It tries to ignore nans, but by doing so it implicitly converts the intermediate values to floats.

I guess it might make sense to split the ScalarOps: One for floats (where we have an identity) and one for ints (where we don't). And then use the nan-check only for floats.

edit

This was introduced here: aesara-devs/aesara#297

@ricardoV94
Copy link
Member

ricardoV94 commented May 14, 2024

The conversion story makes sense. Regarding infty, the code actually initializes those to zero when it's (u)integers.

The number thing was not a hard cutoff. It starts working with larger values and then fails again. In case that helps.

And yes this bug is likely there for a while but was hidden when the max/min could be constant folded, triggering the C implementation of MaxAndArgmax instead which just calls numpy C code and handles it correctly.

brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 4, 2025
This is expected to fail, but has started to pass.

I split out the example from the issue cited in the xfail
to a new test, which fails as expected.

However, the original `test_uint` (which isn't exactly the same as the
example on Issue pymc-devs#770) is passing, while it is marked xfail.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 4, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 4, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 4, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 4, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 5, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 5, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 6, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 14, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 14, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 14, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 14, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 14, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 17, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 17, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
brendan-m-murphy added a commit to brendan-m-murphy/pytensor that referenced this issue Feb 17, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
ricardoV94 pushed a commit that referenced this issue Feb 17, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue #770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
Aarsh-Wankar pushed a commit to Aarsh-Wankar/pytensor that referenced this issue Feb 22, 2025
I split this test up to test uint64 separately, since
this is the case discussed in Issue pymc-devs#770. I also added
a test for the exact example used in that issue.

The uint dtypes with lower precision should pass.

The uint64 case started passing for me locally on Mac OSX,
but still fails on CI. I'm not sure why this is, but at
least the test will be more specific now if it fails in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend
Projects
None yet
Development

No branches or pull requests

3 participants