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

fix test_activation by lowering threshold + validate eps for check_numeric_gradient #12560

Merged
merged 7 commits into from
Sep 20, 2018
Merged

fix test_activation by lowering threshold + validate eps for check_numeric_gradient #12560

merged 7 commits into from
Sep 20, 2018

Conversation

azai91
Copy link
Contributor

@azai91 azai91 commented Sep 14, 2018 •
edited
Loading

Description

Address problem with #12377 by setting threshold my appropriately. Ran test with 10000 random seeds and did not produce error.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • set activation to 1e-5

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@azai91 azai91 mentioned this pull request Sep 14, 2018
Closed
lebeg
lebeg reviewed Sep 14, 2018
View reviewed changes
tests/python/mkl/test_mkldnn.py
@@ -292,7 +291,7 @@ def check_activation_training(stype):
in_location = [mx.nd.array(data_tmp).tostype(stype)]

test = mx.symbol.Activation(data, act_type="relu")
check_numeric_gradient(test, in_location, numeric_eps=1e-2, rtol=0.16, atol=1e-4)
check_numeric_gradient(test, in_location, numeric_eps=1e-5, rtol=0.16, atol=1e-4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this almost an exact fix as in #12418 that didn't solve the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a significant difference between using 1e-5 vs 1e-6. I commented in #12377. in short, you should never use anything less than 1e-5 as the floats do not have enough precision to calculate the difference in the numerator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the explanation!

@azai91 azai91 requested a review from szha as a code owner September 14, 2018 17:30
@kalyc
Copy link
Contributor

kalyc commented Sep 14, 2018

Thanks for your contribution @azai91
Could you update the PR title to be more descriptive?

@mxnet-label-bot[pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 14, 2018
@azai91 azai91 changed the title Fix/test activation fix test_activation by lowering threshold + validate eps for check_numeric_gradient Sep 14, 2018
lebeg
lebeg approved these changes Sep 17, 2018
View reviewed changes
tests/python/mkl/test_mkldnn.py
@@ -292,7 +291,7 @@ def check_activation_training(stype):
in_location = [mx.nd.array(data_tmp).tostype(stype)]

test = mx.symbol.Activation(data, act_type="relu")
check_numeric_gradient(test, in_location, numeric_eps=1e-2, rtol=0.16, atol=1e-4)
check_numeric_gradient(test, in_location, numeric_eps=1e-5, rtol=0.16, atol=1e-4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the explanation!

@lupesko
Copy link
Contributor

lupesko commented Sep 17, 2018

Flagging for @anirudh2290 @sandeep-krishnamurthy @nswamy for review/merge.

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Sep 20, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit 97a7457 into apache:master Sep 20, 2018
@lebeg lebeg mentioned this pull request Oct 9, 2018
Closed
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@access2rohit access2rohit access2rohit approved these changes

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@lupesko lupesko lupesko approved these changes

@lebeg lebeg lebeg approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@azai91 @kalyc @lupesko @lebeg @sandeep-krishnamurthy @access2rohit @marcoabreu