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

Use arraycontext in tests #135

Merged
merged 8 commits into from
Aug 22, 2022
Merged

Use arraycontext in tests #135

merged 8 commits into from
Aug 22, 2022

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Aug 20, 2022

Ports all the tests to the arraycontext infrastructure.

This is not on top of #118 because it can go in without the rest of sumpy being ported.

@alexfikl
Copy link
Contributor Author

After writing most of this, I realized that there's some small conflicts with #118. I'm happy to resolve those if this gets in.

@alexfikl alexfikl force-pushed the actx-based-tests branch 3 times, most recently from 88d7246 to 89adfcb Compare August 20, 2022 18:53
test/test_fmm.py Outdated Show resolved Hide resolved
@alexfikl alexfikl force-pushed the actx-based-tests branch 4 times, most recently from 5cb55f4 to f2e0696 Compare August 21, 2022 07:22
@alexfikl alexfikl marked this pull request as ready for review August 21, 2022 07:23
@alexfikl
Copy link
Contributor Author

alexfikl commented Aug 21, 2022

@inducer This should be ready for a look!

Most of it is just tedious, but there are a few things worth mentioning:

  • This inherits the array context from boxtree.PyOpenCLArrayContext (instead of the one from arraycontext).
  • There were a couple of tests that used np.random and failed when changing the seed in test_kernels.py.

@inducer
Copy link
Owner

inducer commented Aug 21, 2022

  • There were a couple of tests that used np.random and failed when changing the seed in test_kernels.py.

Do you remember which ones those were? Were they just random tolerance botches or something you thought was indicative of a deeper issue? (If the latter, could you file an issue?)

@inducer inducer enabled auto-merge (rebase) August 21, 2022 23:39
@inducer
Copy link
Owner

inducer commented Aug 21, 2022

This looks good to me, and it's an all-around improvement. Thanks!

@inducer inducer merged commit cd7b54f into inducer:main Aug 22, 2022
@alexfikl alexfikl deleted the actx-based-tests branch August 22, 2022 04:37
@alexfikl
Copy link
Contributor Author

alexfikl commented Aug 22, 2022

Do you remember which ones those were? Were they just random tolerance botches or something you thought was indicative of a deeper issue? (If the latter, could you file an issue?)

@inducer Looking through the failed CIs (e.g. https://github.com/inducer/sumpy/runs/7933601203?check_suite_focus=true)

  • test_kernels::test_translations: failed with assert ([-0.19, -0.8] < -0.2).all()
  • test_kernels::test_p2e2p: failed with assert 3.3 > 3.5

so I would say it's just a case of fixing the tolerances to the previous seeds, but I don't know enough about what the tests are actually testing to be certain.

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.

3 participants