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

Add allow_subnormal passthrough to strategies.numpy.from_dtype() for complex values #3558

Merged
merged 5 commits into from Feb 3, 2023

Conversation

felixdivo
Copy link
Contributor

@felixdivo felixdivo commented Jan 25, 2023

I think this was just forgotten.

Related to #3468.

@felixdivo felixdivo marked this pull request as ready for review January 25, 2023 12:00
@felixdivo
Copy link
Contributor Author

Does this need any tests? hypothesis-python/tests/numpy/* never mentiones allow_subnormal in the test files.

@honno
Copy link
Member

honno commented Jan 26, 2023

I think you're right we forgot about this, or at least thought it was low-prio at the time given "subnormal complex numbers" is something hopefully no one has to think about haha. Thanks for your contribution!

Does this need any tests? hypothesis-python/tests/numpy/* never mentiones allow_subnormal in the test files.

I think generally we need tests for subnormal support in extra.numpy... but it's pretty annoying to test, as I don't think we can realistically test for both FTZ and non-FTZ scenarios on the same run of tests 😅 In any case, for extra.array_api we have

def test_subnormal_elements_validation(xp, xps):
"""Strategy with subnormal elements strategy is correctly validated.
For FTZ builds of array modules, a helpful error should raise. Conversely,
for builds of array modules which support subnormals, the strategy should
generate arrays without raising.
"""
elements = {
"min_value": 0.0,
"max_value": width_smallest_normals[32],
"exclude_min": True,
"exclude_max": True,
"allow_subnormal": True,
}
strat = xps.arrays(xp.float32, 10, elements=elements)
if flushes_to_zero(xp, width=32):
with pytest.raises(InvalidArgument, match="Generated subnormal float"):
strat.example()
else:
strat.example()

and

@pytest.mark.parametrize(
"kwargs",
[
{},
{"min_value": -1},
{"max_value": 1},
{"min_value": -1, "max_value": 1},
],
)
def test_subnormal_generation(xp, xps, kwargs):
"""Generation of subnormals is dependent on FTZ behaviour of array module."""
strat = xps.from_dtype(xp.float32, **kwargs).filter(lambda n: n != 0)
if flushes_to_zero(xp, width=32):
assert_no_examples(strat, lambda n: -smallest_normal < n < smallest_normal)
else:
find_any(strat, lambda n: -smallest_normal < n < smallest_normal)

(FYI flushes_to_zero() checks if the given array module flushing to zero at the time, where xp could be np, and xps could be hypothesis.extra.numpy (nps))

Feel free to take a stab at copying these tests, and extending them for complex numbers too, but I'll try to sit down and properly think about this over the weekend.

@felixdivo
Copy link
Contributor Author

"subnormal complex numbers" is something hopefully no one has to think about haha

I also hope so 😆

I'm honestly not too eager to add the tests for this ... 😅 If you will have a look anyways, then I will cowardly retreat here.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 26, 2023

Seconding @honno here, I think we just forgot 😅. And in case our pretty pedantic review process made you wonder, we really deeply appreciate all your PRs! Thanks so much for helping us to improve Hypothesis 😍

@felixdivo
Copy link
Contributor Author

😁 Nice! Your framework is awesome btw! It's an honour to contribute to it. You aim of "pushing software quality" of the Python ecosystem may sound a bit ambitious but I think it aptly describes your impact. 🥳

And yeah, just a few weeks ago I realized that I'd need complex numbers in my research (never would have expected that) and was suprised how much support for it was already lurking in python, hypothesis, numpy, pytorch, ... So I figures I'd do the "last" steps here for everyone to be able to fully use it in my project for testing some numerical algorithms.

@felixdivo
Copy link
Contributor Author

I'm a bit confused regarding the hypothesis-python/RELEASE.rst merge conflict: Should I resolve it by putting my changes in that file only?

@honno
Copy link
Member

honno commented Jan 27, 2023

I'm a bit confused regarding the hypothesis-python/RELEASE.rst merge conflict: Should I resolve it by putting my changes in that file only?

Yeah the auto-release process messed up somehow as RELEASE.rst shouldn't be in the main repo, so you can leave this file be for now.

@felixdivo felixdivo changed the title Add allow_subnormal passthrough to strategies.numpy.from_dtype for complex values Add allow_subnormal passthrough to strategies.numpy.from_dtype() for complex values Jan 27, 2023
@felixdivo
Copy link
Contributor Author

Once this is merged, I'll add the pass-through of width, min_magnitude & max_magnitude.

@honno
Copy link
Member

honno commented Feb 2, 2023

I'll try to sit down and properly think about this over the weekend.

Had something come up so just to say I'll have another go this weekend 😅

@honno
Copy link
Member

honno commented Feb 3, 2023

So I introduced basic tests in test_from_dtype for subnormal validation, which I think are satisfactory.

I noticed this PR currently doesn't update the following helpful error logic for FTZ subnormal behaviour

if self.dtype.kind == "f": # pragma: no cover

Would be nice to update this, even if it is just a nicety (a less-specific helpful error should raise anyway right now), so will explore. It does feel a bit mute given we're not going to test this on CI or expect users/ourselves to have to work out how to build FTZ NumPy (where in the array_api case, my default local build of CuPy had FTZ float32s so I could test this easily).

We also didn't update this behaviour for extra.array_api...

@felixdivo
Copy link
Contributor Author

So I introduced basic tests in test_from_dtype for subnormal validation, which I think are satisfactory.

Nice! 🧑‍🔬

noticed this PR currently doesn't update the following helpful error logic for FTZ subnormal behaviour

That's true, but isn't that already handled by the function, since it eventually calls floats() anyways? And floats() does the following:

if allow_subnormal and next_up(0.0, width=width) == 0: # pragma: no cover
# Not worth having separate CI envs and dependencies just to cover this branch;
# discussion in https://github.com/HypothesisWorks/hypothesis/issues/3092
#
# Erroring out here ensures that the database contents are interpreted
# consistently - which matters for such a foundational strategy, even if it's
# not always true for all user-composed strategies further up the stack.
from _hypothesis_ftz_detector import identify_ftz_culprits
try:
ftz_pkg = identify_ftz_culprits()
except Exception:
ftz_pkg = None
if ftz_pkg:
ftz_msg = (
f"This seems to be because the `{ftz_pkg}` package was compiled with "
f"-ffast-math or a similar option, which sets global processor state "
f"- see https://simonbyrne.github.io/notes/fastmath/ for details. "
f"If you don't know why {ftz_pkg} is installed, `pipdeptree -rp "
f"{ftz_pkg}` will show which packages depend on it."
)
else:
ftz_msg = (
"This is usually because something was compiled with -ffast-math "
"or a similar option, which sets global processor state. See "
"https://simonbyrne.github.io/notes/fastmath/ for a more detailed "
"writeup - and good luck!"
)
raise FloatingPointError(
f"Got allow_subnormal={allow_subnormal!r}, but we can't represent "
f"subnormal floats right now, in violation of the IEEE-754 floating-point "
f"specification. {ftz_msg}"
)

I'm honestly not too much into this, for me it already looked like it was working fine. 😅

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Thanks again @felixdivo!

@Zac-HD Zac-HD merged commit 6ecdd55 into HypothesisWorks:master Feb 3, 2023
@felixdivo felixdivo deleted the patch-2 branch February 4, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants