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

[MXNET-507] Set arbitrary dtype for ret_indices in ordering ops #12250

Merged
merged 2 commits into from
Aug 22, 2018

Conversation

sxjscience
Copy link
Member

@sxjscience sxjscience commented Aug 20, 2018

Description

Continue #11134

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

  • Arbitrary dtype of indices for ordering operators, tests
  • For topk, argsort, add the dtype option to specify the type of the output indices, tests
  • Fix the bug that ordering ops do not support kNullOp, tests
  • Fix the doc of topk to state the the elements are sorted.

Comments

  • Currently, the operator do not support to have input dtype=float16.

@szha
Copy link
Member

szha commented Aug 20, 2018

cc'd @ciyongch @pengzhao-intel

@sxjscience sxjscience changed the title [MXNET-507] Set arbitrary dtype for ret_indices in ordering ops [MXNET-507] [WIP] Set arbitrary dtype for ret_indices in ordering ops Aug 21, 2018
@sxjscience sxjscience changed the title [MXNET-507] [WIP] Set arbitrary dtype for ret_indices in ordering ops [MXNET-507] Set arbitrary dtype for ret_indices in ordering ops Aug 21, 2018
@sxjscience sxjscience mentioned this pull request Aug 21, 2018
@leezu
Copy link
Contributor

leezu commented Aug 21, 2018

@sxjscience would it make sense to point out in the documentation that not specifying integer dtype may lead to the issues described in #11134

@sxjscience
Copy link
Member Author

sxjscience commented Aug 21, 2018 via email

@leezu
Copy link
Contributor

leezu commented Aug 25, 2018

@szha @sxjscience it seems this was merged without the documentation being updated.

@szha
Copy link
Member

szha commented Aug 25, 2018

good catch. @sxjscience could you make another PR for the doc update?

XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Sep 19, 2018
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