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

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

Merged
merged 2 commits into from
Mar 20, 2018

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 changed the title Fix buggy type inference in Correlation [MXNET-100] Fix buggy type inference in Correlation Mar 16, 2018
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?

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

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

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

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

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

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

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

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
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
* 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
* 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
* 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
* fix buggy type inference in correlation

* add test for mutual type inference
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants