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

ENH: add crude assert_allclose; use value testing in vecdot #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Nov 26, 2024

No description provided.

@ev-br
Copy link
Contributor Author

ev-br commented Nov 26, 2024

This should fail with array_api_strict until data-apis/array-api-strict#98 lands.
(verified locally that it passes with numpy and fails with array-api-strict; both with --max-examples 10000)

@ev-br ev-br force-pushed the vecdot_assert_allclose branch from 1e58129 to c8309c8 Compare November 26, 2024 12:31
@asmeurer
Copy link
Member

The CI here passes. It might be because the CI doesn't run enough examples by default and didn't catch the error.

@asmeurer
Copy link
Member

It would be a good idea to run this with a lot of examples. My worry is that we will see the same sorts of flaky falure issues we saw with #168 due to loss of significance unless we filter out ill conditioned inputs (as in #290).

Of course, we can always just merge this and revert it or fix it if issues crop up. The CI here doesn't run many examples relatively speaking, but the CI for array-api-strict and array-api-compat is where you usually find the errors, because they have such a large matrix that they rarer errors are very likely to show up (that is assuming the test in question isn't already being XFAILed for some reason).

@@ -599,3 +600,18 @@ def assert_array_elements(
at_expected = expected[idx]
msg = msg_template.format(sh.fmt_idx(out_repr, idx), at_out, at_expected)
assert at_out == at_expected, msg


def assert_allclose(actual, desired, *, atol=1e-7, rtol=1e-7, equal_nan=True, msg_extra=""):
Copy link
Member

Choose a reason for hiding this comment

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

There's also some similar helpers in test_operators_and_elementwise_functions.py (that file has some of the more complex helper functions, since most of the tests are very similar).

@ev-br
Copy link
Contributor Author

ev-br commented Nov 26, 2024

An alternative strategy, in view of #314 (comment), could be to only test "exact" things in hypothesis (rename assert_equal to assert_equal_if_int or some such) and add fp value testing only for specific functions with fixed inputs, as regular regression tests without hypothesis.

@asmeurer
Copy link
Member

Yes, it's a possibility. It's not something we've really done yet, but maybe we should think about it (see also #284). Non-hypothesis tests have their own issues, which is that they only test specific things. The hypothesis tests have been pretty good at checking a wide range of cases which would definitely be missed with traditional testing. I would definitely strongly encourage the majority of the test suite to continue to use hypothesis. Even something simple like a function with n keyword arguments has something like $2^n$ possible combinations of inputs, and in many cases these combinations do materially affect each other.

And I do agree we should rename functions to be more descriptive of what they are actually doing.

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.

2 participants