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

Relaxing type requirements for broadcast_like #17977

Merged
merged 3 commits into from
May 4, 2020

Conversation

tobecontinued
Copy link
Contributor

Description

Similarly to #14097, broadcast_like op should also not dictate the type of its second argument (since only shape metadata is used there).

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • [x ] Changes are complete (i.e. I finished coding on this PR)
  • [x ] All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • [x ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@mxnet-bot
Copy link

Hey @tobecontinued , 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: [centos-gpu, unix-gpu, miscellaneous, website, edge, unix-cpu, windows-gpu, clang, centos-cpu, windows-cpu, sanity]


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.

@tobecontinued
Copy link
Contributor Author

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

@lanking520 lanking520 added Operator pr-awaiting-review PR is waiting for code review labels Apr 6, 2020
std::vector<int> *out_attrs) {
CHECK_EQ(in_attrs->size(), 2) << " in operator " << attrs.name;
std::vector<int> checked_in_attrs = { (*in_attrs)[0] };
bool ret = !type_is_none((*in_attrs)[1]) &&
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your contribution!

Is it necessary for the condition !type_is_none((*in_attrs)[1]) ?

Copy link
Contributor Author

@tobecontinued tobecontinued Apr 6, 2020

Choose a reason for hiding this comment

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

Well, I actully just found the issue, the code is copied from merged PR #14097, which had a similar issue. I think it is better to keep the same assumpation for all *_like op need to relax type requirment.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is necessary - what FInferType returns is whether it succeeded in inferring all the types (so that if all operators return true we know that all types are inferred). That is why it is important to not lie and return true only if all types are really inferred (even if we do not actually do anything with the other type).

tests/python/unittest/test_operator.py Show resolved Hide resolved
@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu, sanity, centos-cpu, unix-cpu, windows-gpu, windows-cpu, clang, website, edge, centos-gpu, miscellaneous]

@tobecontinued
Copy link
Contributor Author

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu, sanity, unix-cpu, centos-cpu, centos-gpu, miscellaneous, windows-gpu, edge, clang, windows-cpu, website]

@wkcn
Copy link
Member

wkcn commented May 4, 2020

@mxnet-bot run ci [windows-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-gpu]

@wkcn wkcn merged commit 8c76631 into apache:master May 4, 2020
@wkcn
Copy link
Member

wkcn commented May 4, 2020

The PR has been merged.

Thanks for your contribution! : )

AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
* Relaxing type requirements for broadcast_like

* enhance unit test
samskalicky pushed a commit to samskalicky/incubator-mxnet that referenced this pull request Oct 29, 2020
* Relaxing type requirements for broadcast_like

* enhance unit test
samskalicky pushed a commit to samskalicky/incubator-mxnet that referenced this pull request Oct 29, 2020
* Relaxing type requirements for broadcast_like

* enhance unit test
szha pushed a commit that referenced this pull request Oct 29, 2020
* Relaxing type requirements for broadcast_like

* enhance unit test

Co-authored-by: wicky <tbc.dengwenqi@gmail.com>
leezu pushed a commit that referenced this pull request Oct 29, 2020
* Relaxing type requirements for broadcast_like

* enhance unit test

Co-authored-by: wicky <tbc.dengwenqi@gmail.com>
josephevans pushed a commit to josephevans/mxnet that referenced this pull request Dec 8, 2020
…19447)

* Relaxing type requirements for broadcast_like

* enhance unit test

Co-authored-by: wicky <tbc.dengwenqi@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants