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

[test] Expand the tolerance for testing on metal backend #1779

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

Rullec
Copy link
Contributor

@Rullec Rullec commented Aug 25, 2020

Related issue = Close #1679

As @archibate adviced, I expand the approx value on metal platform.

Now test_ad_basic.py can work properly on my machine!

[Click here for the format server]


@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #1779 into master will decrease coverage by 0.93%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1779      +/-   ##
==========================================
- Coverage   43.59%   42.66%   -0.94%     
==========================================
  Files          44       44              
  Lines        5968     6174     +206     
  Branches     1074     1075       +1     
==========================================
+ Hits         2602     2634      +32     
- Misses       3212     3384     +172     
- Partials      154      156       +2     
Impacted Files Coverage Δ
python/taichi/testing.py 75.75% <0.00%> (-1.67%) ⬇️
python/taichi/lang/ops.py 43.89% <0.00%> (-9.09%) ⬇️
python/taichi/lang/expr.py 61.95% <0.00%> (-5.50%) ⬇️
python/taichi/lang/matrix.py 64.73% <0.00%> (-4.59%) ⬇️
python/taichi/lang/snode.py 65.83% <0.00%> (-4.44%) ⬇️
python/taichi/lang/impl.py 64.50% <0.00%> (-2.07%) ⬇️
python/taichi/lang/transformer.py 80.92% <0.00%> (-0.69%) ⬇️
python/taichi/main.py 23.57% <0.00%> (-0.39%) ⬇️
python/taichi/lang/util.py 31.36% <0.00%> (-0.38%) ⬇️
python/taichi/lang/kernel.py 70.96% <0.00%> (-0.14%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c39e84...ee8f1b1. Read the comment docs.

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

Thanks this is great! Sorry it just occurred to me, but could you try running with fast_math=False (without changes in this PR)?

@Rullec
Copy link
Contributor Author

Rullec commented Aug 26, 2020

Thanks this is great! Sorry it just occurred to me, but could you try running with fast_math=False (without changes in this PR)?

Thanks for the reply!
Just now I tried the fast_math=False option by

@if_has_autograd
@ti.all_archs_with(fast_math=False)
def test_trigonometric():
    # grad_test(lambda x: ti.tanh(x), lambda x: np.tanh(x))
    grad_test(lambda x: ti.sin(x), lambda x: np.sin(x))
    # grad_test(lambda x: ti.cos(x), lambda x: np.cos(x))
    # grad_test(lambda x: ti.acos(x), lambda x: np.arccos(x))
    # grad_test(lambda x: ti.asin(x), lambda x: np.arcsin(x))

And this test can still be wrong if I didn't expand the approx in testing.py. Log shown below:

>       assert y[0] == approx(npfunc(v))
E       assert 0.23188334703445435 == 0.2318703549116868 ± 2.3e-07
E        +  where 0.2318703549116868 ± 2.3e-07 = approx(0.2318703549116868)
E        +    where 0.2318703549116868 = <function test_trigonometric.<locals>.<lambda> at 0x111369598>(0.234)

test_ad_basics.py:47: AssertionError

Can I do more tests for you? Maybe this problem shouldn't be ignored in this way easily ;)

@k-ye
Copy link
Member

k-ye commented Aug 26, 2020

I see.

Can I do more tests for you?

No that's the only thing i could think of... Thanks!

Maybe this problem shouldn't be ignored in this way easily ;)

Yeah, but given it didn't fail on my side, I'm attributing this to hardware differences.. ;-(

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] Test test_ad_basic.py failed on MacOS Mojave
2 participants