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

Fix SliceChannel Type inference#16748

Merged
anirudh2290 merged 3 commits intoapache:masterfrom
anirudh2290:fix_amp_slice_channel
Nov 8, 2019
Merged

Fix SliceChannel Type inference#16748
anirudh2290 merged 3 commits intoapache:masterfrom
anirudh2290:fix_amp_slice_channel

Conversation

@anirudh2290
Copy link
Member

@anirudh2290 anirudh2290 commented Nov 7, 2019 •
edited
Loading

Description

Fix SliceChannel Type Inference. Do forward and backward inference for slice channel with ElemwiseAttr logic. Remove exception thrown from infer_type logic in slice channel.
Adds some gluoncv models to AMP examples.

@ptrendx

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 my best 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

ChaiBapchya
ChaiBapchya reviewed Nov 7, 2019
View reviewed changes
src/operator/slice_channel-inl.h Outdated
bool InferType(std::vector<int> *in_type,
std::vector<int> *out_type,
std::vector<int> *aux_type) const override {
/*
Copy link
Contributor

@ChaiBapchya ChaiBapchya Nov 7, 2019

Choose a reason for hiding this comment

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

If this is to be removed, why is it commented? Or is this temporary?

Copy link
Member Author

@anirudh2290 anirudh2290 Nov 7, 2019

Choose a reason for hiding this comment

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

yes, it should have been removed. Good catch, fixed.

ChaiBapchya
ChaiBapchya reviewed Nov 7, 2019
View reviewed changes
tests/python/gpu/test_contrib_amp.py
data2._set_attr(__dtype__="-1")
concat_res = mx.sym.concat(data, data2)
out = mx.sym.split(concat_res, axis=1, num_outputs=2)
final_res = amp.convert_symbol(out)
Copy link
Contributor

@ChaiBapchya ChaiBapchya Nov 7, 2019

Choose a reason for hiding this comment

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

Any checks or assertions needed?

Copy link
Member Author

@anirudh2290 anirudh2290 Nov 7, 2019

Choose a reason for hiding this comment

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

no, this was earlier failing in the convert_symbol call before this change. Just need to check if convert_symbol completes successfully.

ChaiBapchya
ChaiBapchya reviewed Nov 7, 2019
View reviewed changes
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Minor questions.. Also address CI failures..

@anirudh2290 anirudh2290 force-pushed the fix_amp_slice_channel branch from d995250 to 86221d0 Compare November 7, 2019 17:45
@ptrendx
Copy link
Member

ptrendx commented Nov 8, 2019

I don't know enough to comment on the changes to the example etc., but the main fix from this PR (InferType in SliceChannel) looks good. @anirudh2290 Searching for First input must have specified type gives many other hits, do you intend to look into those as well?

@anirudh2290
Copy link
Member Author

anirudh2290 commented Nov 8, 2019 •
edited
Loading

@ptrendx how did you come up with that sentence ?

EDIT: Never mind, its the same error message.

@anirudh2290
Copy link
Member Author

anirudh2290 commented Nov 8, 2019

@ptrendx good point about the other places where error is raised. The scope of the PR increased :) . Can we start with this PR first and I will follow up with another PR/PRs for fixes in other ops. Sorry, little busy with other deliverables too and cannot do it immediately.

For the examples, I have tested running the gluon models for conversion and dummy inference in a loop.

@anirudh2290 anirudh2290 mentioned this pull request Nov 8, 2019
Open
18 tasks
@ptrendx
Copy link
Member

ptrendx commented Nov 8, 2019

Sure, having this PR fix just SliceChannel is totally fine.

@anirudh2290 anirudh2290 merged commit a37dcd4 into apache:master Nov 8, 2019
anirudh2290 added a commit to anirudh2290/mxnet that referenced this pull request Nov 13, 2019
@anirudh2290
Fix SliceChannel Type inference (apache#16748)
fc90c4a 
* Refactor elemwise_op_common and change SliceChannel InferType

* Add gluoncv models

* Comment Faster RCNN models
@anirudh2290 anirudh2290 mentioned this pull request Nov 13, 2019
Merged
7 tasks
ptrendx pushed a commit that referenced this pull request Nov 15, 2019
@anirudh2290 @ptrendx
Fix SliceChannel Type inference (#16748) (#16797)
867c98d 
* Refactor elemwise_op_common and change SliceChannel InferType

* Add gluoncv models

* Comment Faster RCNN models
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

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

2 more reviewers

@ChaiBapchya ChaiBapchya ChaiBapchya left review comments

@nswamy nswamy nswamy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anirudh2290 @ptrendx @nswamy @ChaiBapchya