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

Fix relative difference scala #14417

Merged
merged 4 commits into from
Mar 13, 2019
Merged

Conversation

kiendang
Copy link
Contributor

@kiendang kiendang commented Mar 13, 2019

Description

Fix #14402 where division by zero occurs in relative difference calculation, making arange tests fail.
Solution: if diff is near 0f, i.e < Float.MinPositiveValue just return it instead of dividing by norm.

Also increase number of test cases to 10000 add test cases where arange produces NDArray of [0.0f] to catch this. Test is not flaky anymore.

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

#14413 @lanking520 @perdasilva

@@ -335,7 +335,7 @@ class NDArraySuite extends FunSuite with BeforeAndAfterAll with Matchers {
}

test("arange") {
for (i <- 0 until 5) {
for (i <- 0 until 10000) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of increasing the number of iterations, could you maybe add a test case that specifically hits the behaviour you're trying to circumvent here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That's definitely a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@lanking520 @nswamy could you give me a hand here and review the scala code?

@marcoabreu
Copy link
Contributor

Thanks a lot for your contribution - nice catch!

@kiendang
Copy link
Contributor Author

Oops

Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

LGTM

@lanking520 lanking520 merged commit 82504ad into apache:master Mar 13, 2019
@kiendang kiendang deleted the scala-reldiff branch March 13, 2019 23:34
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
* Fix relative difference scala

* Increase number of cases for scala arange test

* Add cases where arange produces NDArray of [0]

* Remote whitespace
nswamy pushed a commit that referenced this pull request Apr 5, 2019
* Fix relative difference scala

* Increase number of cases for scala arange test

* Add cases where arange produces NDArray of [0]

* Remote whitespace
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Fix relative difference scala

* Increase number of cases for scala arange test

* Add cases where arange produces NDArray of [0]

* Remote whitespace
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.

Scala arange Flaky test
3 participants