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

[MXNET-100] Fix buggy type inference in Correlation#10135

Merged
piiswrong merged 2 commits intoapache:masterfrom
haojin2:fp16_correlation
Mar 20, 2018
Merged

[MXNET-100] Fix buggy type inference in Correlation#10135
piiswrong merged 2 commits intoapache:masterfrom
haojin2:fp16_correlation

Conversation

@haojin2
Copy link
Contributor

@haojin2 haojin2 commented Mar 16, 2018

Description

There was a bug in InferType function in PR #10125, which may cause the type inference to fail under some cases.

Checklist

Essentials

  • Passed code style checking (make lint)
  • 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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Correction of InferType function for Correlation operator

Comments

@haojin2 haojin2 requested a review from cjolivier01 as a code owner March 16, 2018 18:43
@haojin2 haojin2 changed the title Fix buggy type inference in Correlation [MXNET-100] Fix buggy type inference in Correlation Mar 16, 2018
cjolivier01
cjolivier01 reviewed Mar 16, 2018
View reviewed changes
Copy link
Member

@cjolivier01 cjolivier01 left a comment

Choose a reason for hiding this comment

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

this seems strange — are you setting dtype over and over? if so, why?
what’s the prototype for type_assign again?

@haojin2
Copy link
Contributor Author

haojin2 commented Mar 16, 2018

@cjolivier01 https://github.com/apache/incubator-mxnet/blob/92d79f26a5c2914f2087ca1360c0b2090f22014f/src/operator/operator_common.h#L185

reminisce
reminisce reviewed Mar 16, 2018
View reviewed changes
src/operator/correlation-inl.h
type_assign(&(*out_type)[0], dtype);
type_assign(&(*out_type)[1], dtype);
type_assign(&(*out_type)[2], dtype);
type_assign(&dtype, (*in_type)[1]);
Copy link
Contributor

@reminisce reminisce Mar 16, 2018

Choose a reason for hiding this comment

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

I think you can just call ElemwiseType<2, 3>(attrs, in_attrs, out_attrs) to achieve the same purpose.

Copy link
Contributor Author

@haojin2 haojin2 Mar 16, 2018

Choose a reason for hiding this comment

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

This function does not have attrs argument, I guess what we have here is the only way to go.

Copy link
Member

@cjolivier01 cjolivier01 Mar 16, 2018

Choose a reason for hiding this comment

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

is it just trying to set all of the inputs to the first non -1 output type?

Copy link
Contributor Author

@haojin2 haojin2 Mar 16, 2018

Choose a reason for hiding this comment

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

Yes, all input and output types to the inferred type.

Copy link
Member

@cjolivier01 cjolivier01 Mar 16, 2018

Choose a reason for hiding this comment

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

sorry your statement there is kind of vague. can you please explain what you’re trying to do here? pretend i am 5 years old...

Copy link
Member

@cjolivier01 cjolivier01 Mar 16, 2018

Choose a reason for hiding this comment

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

also, you can just pass a blank NodAttrs if necessary

Copy link
Member

@cjolivier01 cjolivier01 Mar 16, 2018

Choose a reason for hiding this comment

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

also, you can just pass a blank NodAttrs if necessary

marcoabreu
marcoabreu suggested changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

Please add a test to catch this problem

@haojin2
Copy link
Contributor Author

haojin2 commented Mar 19, 2018

@marcoabreu test added for mutual type inference in Correlation operator

@piiswrong piiswrong merged commit 048e362 into apache:master Mar 20, 2018
ashokei pushed a commit to ashokei/incubator-mxnet that referenced this pull request Mar 27, 2018
@haojin2 @ashokei
[MXNET-100] Fix buggy type inference in Correlation (apache#10135)
c7e164f 
* fix buggy type inference in correlation

* add test for mutual type inference
jinhuang415 pushed a commit to jinhuang415/incubator-mxnet that referenced this pull request Mar 30, 2018
@haojin2
[MXNET-100] Fix buggy type inference in Correlation (apache#10135)
356935e 
* fix buggy type inference in correlation

* add test for mutual type inference
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@haojin2 @piiswrong
[MXNET-100] Fix buggy type inference in Correlation (apache#10135)
a357dd6 
* fix buggy type inference in correlation

* add test for mutual type inference
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@haojin2 @zheng-da
[MXNET-100] Fix buggy type inference in Correlation (apache#10135)
3e87257 
* fix buggy type inference in correlation

* add test for mutual type inference
@szha szha mentioned this pull request Jul 2, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@cjolivier01 cjolivier01 cjolivier01 left review comments

@marcoabreu marcoabreu marcoabreu requested changes

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@reminisce reminisce reminisce 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.

6 participants

@haojin2 @anirudh2290 @reminisce @cjolivier01 @marcoabreu @piiswrong