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

allclose detail + misc tests improvements #457

Merged
merged 16 commits into from
Jul 25, 2022

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Jul 18, 2022

This PR started out to just improve allclose output, but grew to encompass a few other test improvements/simplications I have been meaning to get to:

  • Moves integration test dir test_tools to utils. This makes it easier to distinguish from tests, and with pytest it is just generally good to reserve the prefix test_ exclusively for test modules
  • Remove the utils.asserts module. In general it is better if helper functions only return results of some sort, and leave assertions to happen only in actual tests. I changed the tests that were using these functions to make their own assertions inside tests
  • A few other test modules with helper functions inside were updated similarly. This will be an ongoing update process. The end result will help most of the tests adhere to a standard "Assemble, Act, Assert" structure.
  • A new utils.comparison.allclose has been added [1] that prints more detailed information about differences:
    tests/integration/test_jacobi.py:58: AssertionError
    ----------------------------- Captured stdout call -----------------------------
    First 1 difference for allclose (with diff_limit=5):
    
     index (0,): 1261.56396484375 1262.56396484375
    =============================== warnings summary ===============================
    
    By default diff_limit is 5 but this can be increased or set to None for no limit.
  • Tests have been updated to use the new allclose throughout.
  • A few other modules that had monolithic tests have been split up, parametrized, etc.

Commits are broken out discretely in case that is simpler to review.

[1] This new allclose uses np.atleast_1d so it would really be good to get the small PR #404 merged, if possible, before this one.

Copy link
Contributor

@manopapad manopapad left a comment

Choose a reason for hiding this comment

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

LGTM. I will be tackling #404 next, if you prefer to hold off on merging until that one, so you can switch to using cunumeric.atleast1d in this same PR.

@bryevdv
Copy link
Contributor Author

bryevdv commented Jul 19, 2022

Thanks I will wait on #404!

@bryevdv
Copy link
Contributor Author

bryevdv commented Jul 21, 2022

merged branch-22.07 in 6960147 to resolve conflicts from #466

@bryevdv bryevdv merged commit 63a8cf0 into nv-legate:branch-22.07 Jul 25, 2022
@bryevdv bryevdv deleted the bryanv/allclose branch July 25, 2022 16:00
jjwilke pushed a commit to jjwilke/cunumeric that referenced this pull request Jul 29, 2022
* rename tests_tools -> utils

* parametrize window tests

* remove superfluous asserts module

* use utils.allclose

* update test_view.py to AAA

* parametrize unary ufunc tests

* paramterize test_trilu.py

* parametrize test_transform.py

* simplify test_squeeze.py, update to AAA

* Apply suggestions from code review

* use ~close insead of not close

* fix transpose test

* formatting

* typo

* Update tests/integration/utils/comparisons.py
jjwilke pushed a commit to jjwilke/cunumeric that referenced this pull request Jul 29, 2022
* rename tests_tools -> utils

* parametrize window tests

* remove superfluous asserts module

* use utils.allclose

* update test_view.py to AAA

* parametrize unary ufunc tests

* paramterize test_trilu.py

* parametrize test_transform.py

* simplify test_squeeze.py, update to AAA

* Apply suggestions from code review

* use ~close insead of not close

* fix transpose test

* formatting

* typo

* Update tests/integration/utils/comparisons.py
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