Skip to content

Subnormals make testing flush-to-zero libs/builds impractical #42

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

Closed
leofang opened this issue Nov 16, 2021 · 17 comments · Fixed by #45
Closed

Subnormals make testing flush-to-zero libs/builds impractical #42

leofang opened this issue Nov 16, 2021 · 17 comments · Fixed by #45

Comments

@leofang
Copy link

leofang commented Nov 16, 2021

Running the test suite against cupy.array_api I'm seeing a lot of the following error:

self = <hypothesis.extra.array_api.ArrayStrategy object at 0x7fd585346f40>, val = 5.877471754111438e-39, val_0d = Array(0., dtype=float32)
strategy = floats(width=32)

    def check_set_value(self, val, val_0d, strategy):
        finite = self.builtin is bool or self.xp.isfinite(val_0d)
        if finite and self.builtin(val_0d) != val:
>           raise InvalidArgument(
                f"Generated array element {val!r} from strategy {strategy} "
                f"cannot be represented with dtype {self.dtype}. "
                f"Array module {self.xp.__name__} instead "
                f"represents the element as {val_0d}. "
                "Consider using a more precise elements strategy, "
                "for example passing the width argument to floats()."
            )
E           hypothesis.errors.InvalidArgument: Generated array element 5.877471754111438e-39 from strategy floats(width=32) cannot be represented with dtype float32. Array module cupy.array_api instead represents the element as 0.0. Consider using a more precise elements strategy, for example passing the width argument to floats().

I think the comparison self.builtin(val_0d) != val should be changed to isclose(val_0d, val).

@rgommers
Copy link
Member

@honno could you have a look at this?

@leofang can we move this issue to the https://github.com/data-apis/array-api-tests repo?

@honno
Copy link
Member

honno commented Nov 16, 2021

Will investigate, just noting this is actually a problem with Hypothesis.

@leofang leofang transferred this issue from data-apis/array-api Nov 16, 2021
@leofang
Copy link
Author

leofang commented Nov 16, 2021

lol Thanks @rgommers, I clicked the wrong link and didn't pay attention... I transferred the issue.

@honno
Copy link
Member

honno commented Nov 17, 2021

For folks interested, the problem here is probably to do with issue HypothesisWorks/hypothesis#3153. If we can't get it fixed soon then I know a temporary solution for the test suite :)

Something fun I found with how NumPy and CuPy differ in how it represents an unpresentable float

>>> from cupy import array_api as cxp
>>> x1 = cxp.asarray(1.e-45, dtype=cxp.float32)
>>> x1 > 0
Array(False, dtype=bool)
>>> from numpy import array_api as nxp
>>> x2 = nxp.asarray(1.e-45, dtype=nxp.float32)
>>> x2 > 0
Array(True, dtype=bool)

(but to clarify, the problem is with Hypothesis generating these unpresentable floats in the first place)

@honno
Copy link
Member

honno commented Nov 17, 2021

@leofang If you'd like to try the #43 branch at honno/array-api-tests/tree/float-width-patch—resolves this on my end at least 😅

@Zac-HD
Copy link

Zac-HD commented Nov 17, 2021

These are not unrepresentable floats; they're IEEE 754 standard "subnormals" - and exposing the fact that cupy.array_api is flushing subnormals to zero maybe due to a compiler flag. This is a pretty common standards violation (it also helps performance on Intel CPIs, for example) and the array API standard might change to allow it, but it is currently a violation.

We should improve that Hypothesis error message to make this clear, though.

@honno
Copy link
Member

honno commented Nov 17, 2021

Opened data-apis/array-api#339 to see if the spec could change (but in any case both @asmeurer and I are on-board that the test suite shouldn't test with subnormals).

@Zac-HD
Copy link

Zac-HD commented Nov 17, 2021

Sounds like sufficient cause to add floats(..., allow_subnormal=True) to Hypothesis and plumb it through the various array strategies then, open an issue and/or PR?

@honno honno changed the title Improper comparison of floating point numbers Subnormals make testing flush-to-zero libs/builds impractical Nov 17, 2021
@honno
Copy link
Member

honno commented Nov 17, 2021

Sounds like sufficient cause to add floats(..., allow_subnormal=True) to Hypothesis and plumb it through the various array strategies then, open an issue and/or PR?

@Zac-HD ah I'll write a quick issue now.

@leofang
Copy link
Author

leofang commented Nov 18, 2021

@Zac-HD good finding!

@Zac-HD
Copy link

Zac-HD commented Nov 26, 2021

We now have PR up to add an st.floats(allow_subnormal=False) option to Hypothesis, and plan automatically enable this for Array API libraries with FTZ set, as well as improving the error message for users who bypass that. See HypothesisWorks/hypothesis#3156 (review) 😃

@honno
Copy link
Member

honno commented Nov 29, 2021

Another fun thing with CuPy:

>>> from cupy import array_api as cxp
>>> cxp.asarray(2.225073858507201e-308, dtype=cxp.float64) == 0  # largest float64 subnormal
Array(False, dtype=bool)
>>> cxp.asarray(1.1754942106924411e-38, dtype=cxp.float32) == 0  # largest float32 subnormal
Array(True, dtype=bool)

@leofang not a biggie, but could be good to know what's happening here.

EDIT: Ah so it could be ftz for float32, but subnormal compliance for float64. Will make sure that carries over to Hypothesis.

@leofang
Copy link
Author

leofang commented Dec 2, 2021

Ah, good finding @honno, yes it's specified by the CUDA documentation (so it's not controlled by CuPy):

The flags have no effect on double precision or on devices of compute capability below 2.0.

https://docs.nvidia.com/cuda/floating-point/index.html#compiler-flags

@honno
Copy link
Member

honno commented Dec 3, 2021

The test suite should now work fine with FTZ libraries 😁 Only checked this locally with CuPy upstream, but we're defo interested in getting a CI job running... we just need to tidy up CI generally first 😅

Thanks for raising this issue @leofang!

EDIT: Note there are some test cases that still fail with subnormals, but that's not because they can't be represented (e.g. float64 on my build of CuPy does not flush-to-zero). I have no idea what's going on in a tests like test_log myself—probably a good time to reimplement some things and get some user-friendly error messages working.

@honno
Copy link
Member

honno commented Dec 3, 2021

EDIT: Note there are some test cases that still fail with subnormals, but that's not because they can't be represented (e.g. float64 on my build of CuPy does not flush-to-zero). I have no idea what's going on in a tests like test_log myself—probably a good time to reimplement some things and get some user-friendly error messages working.

We'll wait on data-apis/array-api#339 to resolve. If the consortium decides subnormals out-of-scope (probably will, right?), I'll just disable subnormals completely. It's good Hypothesis still supports them even if they're out-of-scope, but wouldn't be fit for the test suite.

@leofang
Copy link
Author

leofang commented Dec 5, 2021

Many thanks @honno for the amazing efforts on both the hypothesis and array-api-tests sides and @Zac-HD for the help!

cc: @kmaehashi for vis

@honno
Copy link
Member

honno commented Dec 8, 2021

EDIT: Note there are some test cases that still fail with subnormals, but that's not because they can't be represented (e.g. float64 on my build of CuPy does not flush-to-zero). I have no idea what's going on in a tests like test_log myself—probably a good time to reimplement some things and get some user-friendly error messages working.

We'll wait on data-apis/array-api#339 to resolve. If the consortium decides subnormals out-of-scope (probably will, right?), I'll just disable subnormals completely. It's good Hypothesis still supports them even if they're out-of-scope, but wouldn't be fit for the test suite.

Subnormals are now out-of-scope, so they've been disabled completely in #47.

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