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

Revert default return type for indices in argsort() and topk() to fp32 #15360

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented Jun 25, 2019

Description

reverts breaking change introduced in #15170

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

@marcoabreu
Copy link
Contributor

Is there an issue that tracks the problem? Also, please add a test that will reveal whatever was broken.

@access2rohit
Copy link
Contributor Author

@marcoabreu the test was actually there to check for default behavior. I changed that test in my previous PR(since I was changing default behavior, a breaking change). That's why I am reverting my changes back to previous default behavior. Do you still think I need to add more cases ?

@access2rohit
Copy link
Contributor Author

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

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Jun 26, 2019
@marcoabreu
Copy link
Contributor

I'm having a bit of trouble understanding what exactly broke as result of the change. Is it the pure fact that the default behavior changed (which as you stated, shouldn't happen) or was there a bigger thing?

@apeforest
Copy link
Contributor

@marcoabreu Yes, the breaking change is mainly that the default behavior (default return data type changed from float to integer), which is more correct given that the operator returns the index to an element in the tensor. However, it may break existing script when downstream operators expects a float output from argsort/topk. This was identified in one of the tests of Java language binding. @frankfliu. Nonetheless, we should make this breaking change in MXNet 2.0 but not before that.

@marcoabreu
Copy link
Contributor

Where are these tests from Frank located? Could we bring them either into the repository or add a test as part of this PR with a comment to an issue that we will track in the MXNet 2.0 Todo list?

@apeforest
Copy link
Contributor

@frankfliu could you please point to the test that failed after @access2rohit PR #15170

@access2rohit Please add this API change as TODO in MXNet 2.0. Thanks

@access2rohit
Copy link
Contributor Author

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

@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Jun 28, 2019
@apeforest
Copy link
Contributor

Do you also need to update the script in test_large_array.py? Otherwise the nightly test would still fail?

@access2rohit
Copy link
Contributor Author

access2rohit commented Jun 28, 2019

@apeforest I don't need to do that. These tests are correct. I just updated my PR with correct macro after rerunning the nightly tests for sort, argsort and topk. The only problem was due to incorrect macro being used earlier that I have now fixed.

@lebeg
Copy link
Contributor

lebeg commented Jul 1, 2019

@marcoabreu @apeforest can we merge this?

Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM

@apeforest apeforest merged commit d74b993 into apache:master Jul 1, 2019
access2rohit added a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
access2rohit added a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 25, 2019
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-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants