Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix atol for test_preloaded_multi_sgd #16356

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

Caenorst
Copy link
Contributor

@Caenorst Caenorst commented Oct 2, 2019

Description

Fix atol for test_preloaded_multi_sgd
Following Issue: #16345
Fixes #16122

Checklist

Essentials

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Code is well-documented:

Changes

  • Change atol on fp16 from 1e-3 to 1e-2

Comments

  • As asked on the issue thread, tested 10k times on my machine

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix. @Caenorst
Can you add "Fixes #PR" that would auto close the issue if/when this PR gets merged.

Thanks again for testing it 10k times before the PR!

@@ -396,7 +396,7 @@ def _assert_all_almost_equal(lhs_list, rhs_list, rtol, atol):
assert_almost_equal(lhs.asnumpy(), rhs.asnumpy(), rtol=rtol, atol=atol)
if dtype == 'float16':
rtol = 1e-3
atol = 1e-3
atol = 1e-2
else:
rtol = 1e-5
atol = 1e-6
Copy link
Contributor

Choose a reason for hiding this comment

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

what about when its !float16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing 10k including non fp16, so I guess the other tolerances are fine

@Caenorst
Copy link
Contributor Author

Caenorst commented Oct 2, 2019

Thanks for the quick fix. @Caenorst
Can you add "Fixes #PR" that would auto close the issue if/when this PR gets merged.

Thanks again for testing it 10k times before the PR!

Where should I add this "Fixes #16122 " ?

@ChaiBapchya
Copy link
Contributor

Generally in the description of the PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants