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

[Backport] Fix for test_ndarray.test_order failing on CI (v1.3.x) #12725

Merged
merged 3 commits into from
Oct 8, 2018

Conversation

ankkhedia
Copy link
Contributor

Description

Backporting fix for flaky test: test_ndarray.test_order from master as the test failed in CI. Tracked in #12310

The test uses np.int16 which is not a supported dtype for ndarray.
Also, conversion of np array to ndarray defaults to float32 in destination ndarray even though the dtype of source could be anything leading to failure of unites due to loss of precision in some cases.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Code is well-documented:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

--> Removed tests for np.int16
--> Instead of using default dtype for converting arrays , used the type from source array to convert to target array.

@ankkhedia
Copy link
Contributor Author

@marcoabreu @lebeg

@@ -819,9 +831,9 @@ def get_large_matrix():
# Repeat those tests that don't involve indices. These should pass even with
# duplicated input data values (over many repeated runs with different random seeds,
# this will be tested).
for dtype in [np.int16, np.int32, np.int64, np.float32, np.float64]:
for dtype in [ np.int32, np.int64, np.float32, np.float64]:
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a formatting glitch - whitespace after opening square bracket

@lebeg
Copy link
Contributor

lebeg commented Oct 2, 2018

You have a typo in the title ;) I suggest for the title:
[Backport] Fix for test_ndarray.test_order failing on CI (v1.3.x)

@ankkhedia ankkhedia changed the title [Backport] Fix for testorder failing on CI fro v1.3.x backported from Master [Backport] Fix for test_ndarray.test_order failing on CI (v1.3.x) Oct 2, 2018
@lebeg
Copy link
Contributor

lebeg commented Oct 2, 2018

@ankkhedia strange, I see your commit, but not the fix in the main diff.

@vrakesh
Copy link
Contributor

vrakesh commented Oct 3, 2018

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

@vrakesh
Copy link
Contributor

vrakesh commented Oct 3, 2018

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

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 3, 2018
@ankkhedia
Copy link
Contributor Author

@lebeg Could you please take a look now. So, there were two places with that formatting glitch, one was fixed while other was looked over in previous commit.

@ankkhedia
Copy link
Contributor Author

@lebeg ping!

Copy link
Contributor

@lebeg lebeg left a comment

Choose a reason for hiding this comment

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

Sorry, for late response, lgtm

@ankkhedia
Copy link
Contributor Author

@sandeep-krishnamurthy Could you please review and merge this PR if it looks good.

@anirudh2290 anirudh2290 merged commit 0a286a0 into apache:v1.3.x Oct 8, 2018
@ankkhedia ankkhedia deleted the v1.3.x branch October 8, 2018 21:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge pr-awaiting-testing PR is reviewed and waiting CI build and test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants