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

Fix binary scalar dtype and add bool support #18277

Merged
merged 5 commits into from
May 22, 2020
Merged

Fix binary scalar dtype and add bool support #18277

merged 5 commits into from
May 22, 2020

Conversation

Tommliu
Copy link
Contributor

@Tommliu Tommliu commented May 11, 2020 •
edited
Loading

Description

attempt to fix the issues mentioned in

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 https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

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

@mxnet-bot
Copy link

Hey @Tommliu , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [windows-gpu, website, clang, edge, miscellaneous, centos-cpu, sanity, windows-cpu, unix-cpu, unix-gpu, centos-gpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@Tommliu
Copy link
Contributor Author

Tommliu commented May 11, 2020

@mxnet-bot run ci [centos-cpu, centos-gpu, unix-cpu, unix-gpu, windows-cpu, windows-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-gpu, centos-cpu, windows-cpu, unix-gpu, unix-cpu, centos-gpu]

@Tommliu Tommliu force-pushed the issue-fix branch 3 times, most recently from eeb7e20 to d970243 Compare May 19, 2020 04:04
@Tommliu Tommliu requested a review from szha as a code owner May 19, 2020 04:04
sxjscience
sxjscience reviewed May 19, 2020
View reviewed changes
python/mxnet/symbol/numpy/_symbol.py Outdated
if rfn_scalar is None:
# commutative function
return lfn_scalar(rhs, float(lhs), out=out)
return lfn_scalar(rhs, float(lhs), is_int=is_int, out=out)
Copy link
Member

Choose a reason for hiding this comment

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

is_int=is_int does not seem to be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we add a struct NumpyBinaryScalarParam which contains scalar and is_int to indicate the dtype of scalar input. Using this struct to solve type issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

is_int=is_int does not seem to be necessary

Because of the existence of FFI, we can get the scalar type when writing C++ codes instead of writing Python. The implementation of symbol doesn't contain FFI process, that's why we need to use a parameter "is_int" when calling symbol function.

@Tommliu Tommliu force-pushed the issue-fix branch 2 times, most recently from 11d4d87 to 74fd79e Compare May 20, 2020 08:27
@yzhliu
Copy link
Member

yzhliu commented May 20, 2020

would this also resolve #18068 ?

@sxjscience
Copy link
Member

I think it will resolve the issue. However, the tests are still failing.

@Tommliu
Copy link
Contributor Author

Tommliu commented May 21, 2020

still working on fixing those tests cause these operators are written in many different files

@Tommliu
Copy link
Contributor Author

Tommliu commented May 21, 2020

@yzhliu This pr contains the fix to that issue #18068

@yzhliu yzhliu mentioned this pull request May 21, 2020
Closed
@Tommliu Tommliu force-pushed the issue-fix branch 2 times, most recently from 8a2e9ac to ba0a7f3 Compare May 21, 2020 06:28
@Tommliu Tommliu requested a review from eric-haibin-lin as a code owner May 21, 2020 06:28
@Tommliu Tommliu changed the title Fix binary scalar operator dtype and add bool support for concat Fix binary scalar dtype and add bool support May 21, 2020
@Tommliu Tommliu force-pushed the issue-fix branch 2 times, most recently from 963771e to 139da7d Compare May 21, 2020 09:23
@Tommliu
Copy link
Contributor Author

Tommliu commented May 21, 2020

@mxnet-bot run ci [centos-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [centos-cpu]

@Tommliu
Copy link
Contributor Author

Tommliu commented May 22, 2020

@mxnet-bot run ci [windows-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-gpu]

@yzhliu yzhliu merged commit 48dea6e into apache:master May 22, 2020
@yzhliu
Copy link
Member

yzhliu commented May 22, 2020

Thanks @Tommliu @cassinixu @sxjscience

This was referenced May 22, 2020
Closed
Closed
Closed
Closed
AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
@Tommliu @cassinixu @AntiZpvoh
Fix binary scalar dtype and add bool support (apache#18277)
450b057 
* add boolean support for concatenate (apache#18213)

* fix binary scalar logic dtype (apache#16964)

* common_expr test remove

* binary scalar op support scalar dtype

Co-Authored-By: Wentao Xu <[email protected]>

* test_error_fix

Co-authored-by: Wentao Xu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanwentaoxu ryanwentaoxu ryanwentaoxu left review comments

@sxjscience sxjscience sxjscience approved these changes

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

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Tommliu @mxnet-bot @yzhliu @sxjscience @ryanwentaoxu