Skip to content

Increase coverage and other niceties for the operator/elementwise tests #89

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

Merged
merged 35 commits into from
Feb 3, 2022

Conversation

honno
Copy link
Member

@honno honno commented Jan 26, 2022

This PR:

  • Utilities ndindex.iter_indices() to greatly improve values testing
    • This is used for every test via the newly introduced *_assert_against_refimpl() utils
  • Implement values testing for all remaining op/elwise tests
  • Test scalar scenarios for binary parametrized tests (i.e. the ones that cover both operators and elementwise functions)
  • Greatly improves error output (no more million random variables popping up when there's an error)

Additionally:

@honno
Copy link
Member Author

honno commented Feb 1, 2022

@asmeurer Not quite ready yet, but you might wanna skim over to see if you can identify any issues with the "reference implementation" approach I'm using here... it's a big change 😅

Notably this does still pass-but-filter special cases (and without using boolean masks). I wrote a high-level note here:

# This module tests elementwise functions/operators against a reference
# implementation. We iterate through the input array(s) and resulting array,
# casting the indexed arrays to Python scalars and calculating the expected
# output with `refimpl` function.
#
# This is finicky to refactor, but possible and ultimately worthwhile - hence
# why these *_assert_again_refimpl() utilities exist.
#
# Values which are special-cased are generated and passed, but are filtered by
# the `filter_` callable before they can be asserted against `refimpl`. We
# automatically generate tests for special cases in the special_cases/ dir. We
# still pass them here so as to ensure their presence doesn't affect the outputs
# respective to non-special-cased elements.
#
# By default, results are casted to scalars the same way that the inputs are.
# You can specify a cast via `res_stype, i.e. when a function accepts numerical
# inputs but returns boolean arrays.
#
# By default, floating-point functions/methods are loosely asserted against. Use
# `strict_check=True` when they should be strictly asserted against, i.e.
# when a function should return intergrals.

@asmeurer
Copy link
Member

asmeurer commented Feb 1, 2022

Let's make sure we document that we're doing this and that any errors should usually mean the function is wrong, but it could also mean our tolerances need to be loosened.

@asmeurer
Copy link
Member

asmeurer commented Feb 1, 2022

My biggest worry here is float32, since the reference implementation will operate on float64. And if we ever add a 16-bit float, that will be even worse. You might want to test this on something like cupy just to see if it works on an accelerator.

# when a function should return intergrals.


def isclose(a: float, b: float, rel_tol: float = 0.25, abs_tol: float = 1) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

I guess since we are testing one float at a time, just using math.isclose here is fine. In general though, we will want an isclose, and corresponding assert_approximately_equal method that only use the array API methods so that they can be called on arrays (I will need this for the linalg tests).

I don't think it will be an issue, but if we do run into issues with float32 and the fact that Python float is 64-bit only, we may need to make this use array operations too. That would also give us more control over the exact formula used for "closeness", though I doubt that will be an issue either given our very loose tolerances.

Copy link
Member Author

@honno honno Feb 2, 2022

Choose a reason for hiding this comment

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

In general though, we will want an isclose, and corresponding assert_approximately_equal method that only use the array API methods so that they can be called on arrays (I will need this for the linalg tests).

The *_assert_against_refimpl utils do this along an array/arrays, only using the Array API endpoints of indexing and scalar casting. I've kept this in just the elwise/op file for now as it's quite particular to those tests, but you might want to draw inspiration from it.

I don't think it will be an issue, but if we do run into issues with float32 and the fact that Python float is 64-bit only, we may need to make this use array operations too.

Ah I see, good to keep in mind.

@honno honno marked this pull request as ready for review February 2, 2022 11:54
@honno
Copy link
Member Author

honno commented Feb 2, 2022

Happy with this PR now.

@honno honno mentioned this pull request Feb 2, 2022
@honno honno merged commit 0f63fab into data-apis:master Feb 3, 2022
@honno honno deleted the values-testing branch February 8, 2022 10:05
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.

Test shape broadcasting for _all_ elementwise/operator tests Pretty print indices
2 participants