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

[FEA] Refactor isclose tests. #10284

Closed
bdice opened this issue Feb 14, 2022 · 3 comments
Closed

[FEA] Refactor isclose tests. #10284

bdice opened this issue Feb 14, 2022 · 3 comments
Labels
cuDF (Python) Affects Python cuDF API. feature request New feature or request

Comments

@bdice
Copy link
Contributor

bdice commented Feb 14, 2022

Is your feature request related to a problem? Please describe.
In PR #10203, we discovered there is a pytest parametrization for isclose that is 6x6x6x6 = 1296 tests. #10203 (comment)

We can minimize the number of test cases dramatically while improving our test coverage.

Describe the solution you'd like
We want to cover all of the following cases for "low precision" (1e-5 or more, for unit scale data) and "high precision" (1e-8 or less, for unit-scale data):

  • values are within rtol but not atol (tiny atol, but the scale of the values is large enough to catch it with rtol)
  • values are within atol but not rtol (tiny rtol, scale of values is close to zero)
  • values are outside of rtol and atol (easy case)
  • values are inside of rtol and atol (easy case)
  • different dtypes cast as expected (which requires the "high precision" case above for value scales on the order of 1)

We can parametrize these in a few (4-6) sets of parameters that provide both data and tolerances, instead of the full matrix of all data and tolerance combinations.

Additional context

  • isclose is not commutative between a and b. The formula for isclose(a, b, rtol, atol) is absolute(a - b) <= (atol + rtol * absolute(b)).
  • Real-world use cases of isclose often deal with comparing "small" differences, often at a precision greater than can be represented in a float32. That is, comparisons of 1.00000001 and 1.
    • e.g. np.float32(1.00000001) == 1 (indistinguishable float32 representations) but not np.isclose(1.00000001, 1, rtol=1e-10, atol=1e-10) (these values may be considered non-equal and not-close at some higher precision).
    • We can "defensively" choose our test data such that it would fail if the data were cast as float32 instead of float64.
@bdice bdice added feature request New feature or request Needs Triage Need team to review and classify labels Feb 14, 2022
@github-actions github-actions bot added this to Needs prioritizing in Feature Planning Feb 14, 2022
@bdice bdice added cuDF (Python) Affects Python cuDF API. tech debt and removed Needs Triage Need team to review and classify labels Feb 14, 2022
@bdice bdice added this to Issue-Needs prioritizing in v22.04 Release via automation Feb 14, 2022
@bdice bdice removed this from Needs prioritizing in Feature Planning Feb 14, 2022
@bdice bdice moved this from Issue-Needs prioritizing to Issue-P2 in v22.04 Release Feb 14, 2022
@bdice bdice assigned bdice and unassigned bdice Feb 14, 2022
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@bdice bdice removed this from Issue-P2 in v22.04 Release Mar 18, 2022
@bdice bdice added this to Issue-Needs prioritizing in v22.06 Release via automation Mar 18, 2022
@bdice bdice moved this from Issue-Needs prioritizing to Issue-P2 in v22.06 Release Mar 18, 2022
@bdice bdice removed this from Issue-P2 in v22.06 Release May 24, 2022
@bdice bdice added this to Issue-Needs prioritizing in v22.08 Release via automation May 24, 2022
@bdice bdice removed this from Issue-Needs prioritizing in v22.08 Release May 24, 2022
@bdice bdice added this to Needs prioritizing in Other Issues via automation May 24, 2022
@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@vyasr
Copy link
Contributor

vyasr commented May 15, 2024

I'm going to close this in favor of #13593

@vyasr vyasr closed this as completed May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuDF (Python) Affects Python cuDF API. feature request New feature or request
Projects
No open projects
Other Issues
Needs prioritizing
Development

No branches or pull requests

3 participants