Skip to content

Conversation

@anijain2305
Copy link
Contributor

@FrozenGene @u99127 @jackwish @tqchen

Pinging for review. The code changes will be very small after we have requantize merged in.

@zhenhuaw-me
Copy link
Contributor

Thank you @anijain2305 , please remind me when this PR rebased to a merged requantize head :)

@anijain2305 anijain2305 force-pushed the qnn_concatenate branch 2 times, most recently from 43be829 to b232102 Compare August 8, 2019 23:46
@anijain2305
Copy link
Contributor Author

@u99127 @jackwish This is now ready for review.

Copy link
Contributor

@zhenhuaw-me zhenhuaw-me left a comment

Choose a reason for hiding this comment

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

LGTM.

As I am not community reviewer, you may need someone else too approve, my message is only comment. :)

@anijain2305
Copy link
Contributor Author

@u99127 @tqchen Please review as you get time.

@anijain2305
Copy link
Contributor Author

@u99127 @ZihengJiang @vinx13 Please review. This (hopefully) should be quick.

@vinx13 vinx13 merged commit f06ef4f into apache:master Aug 14, 2019

# Find the dtype of the input expr. This is required for the requantize op. Since, this is
# concatenate op, the dtype of the input is same as dtype of the output.
data0 = relay.transform.infer_type(data[0])
Copy link
Member

@vinx13 vinx13 Aug 15, 2019

Choose a reason for hiding this comment

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

@anijain2305 This API has been removed in favor of InferType, please update and send another PR. Also I'm considering that requantize can by default use same output dtype if null is provided, so that we can avoid type inference here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a PR for fixing InferType. I think it missed CI because CI passed 3 days ago and InferType changes happened after that.

Regarding requantize, it makes sense. Let me think little more tomorrow and finalize this.

@ZihengJiang
Copy link
Contributor

ZihengJiang commented Aug 15, 2019

It seems the new added tests will raise error.

http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-3762/5/pipeline
http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/master/1448/pipeline

Since this breaks CI for other PRs, could you raise a PR to fix it soon? @anijain2305

ZihengJiang added a commit that referenced this pull request Aug 15, 2019
@anijain2305
Copy link
Contributor Author

anijain2305 commented Aug 15, 2019

@ZihengJiang @vinx13 Added the following quick fix for InferType

#3779

wweic pushed a commit to neo-ai/tvm that referenced this pull request Aug 16, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019
@anijain2305 anijain2305 deleted the qnn_concatenate branch November 13, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants