Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design: infer_var_type #4795

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Oct 13, 2017

No description provided.

@QiJune
Copy link
Member

QiJune commented Oct 13, 2017

Maybe we should also register InferShape to OpInfo @jacquesqiao


The variable in our design can hold variant types. Such as `LoDTensor` and `SelectedRows`. An operator should be able to inference the variable types of its output.

For example, a `lookup table` operator takes two `LoDTensor`; one is a float tensor as the embedding table, the other is an int tensor as word ID. The gradient operator of `lookup table` will generate a `SelectedRows` as its output. A `sum` operator can take both `LoDTensor` and `SelectedRows` as its inputs and will generate a `LoDTensor` if any of its inputs is `LoDTensor`, otherwise, the `sum` operator will generate `SelectedRows` as its output.
Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding, SelectedRows is a LoDTensor which stores word embeddings and a LoD which stores the sequence info and a seleted row ids (may be a vector<int>).

the LoDTensor is supported by other ops already.

In my option, the selected row ids is only used by lookup and do not need to be exposed to other ops such as sum, and no necessary to have a new data type that should be processed by ops besides LoDTensor;

Copy link
Contributor

Choose a reason for hiding this comment

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

Had a discussion with reyang, the new type SelectedRow serves in scenarios such as sum the outputs of two lookup.

It seems ok and there is no better way to support this currently.

@reyoung reyoung changed the title Design Doc: infer_var_type Design & implementation: infer_var_type Oct 13, 2017
@reyoung reyoung changed the title Design & implementation: infer_var_type Design: infer_var_type Oct 13, 2017
@reyoung reyoung mentioned this pull request Oct 13, 2017
Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

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

InferVarType and InferShape are actually the same thing -- it should be called "Infer Type"

@reyoung
Copy link
Collaborator Author

reyoung commented Oct 16, 2017

InferVarType and InferShape are actually the same thing -- it should be called "Infer Type"

@wangkuiyi
However, I think there are several differences between InferVarType and InferShape.

  1. InferVarType MUST be invoked before InferShape, since we must set type before we set shape due to our protobuf design.
  2. InferVarType will be invoked ONLY when compiling since it will not be changed at runtime. However, InferShape will be invoked both compile-time and run-time.
  3. Most operators do not need to write InferVarType since they just generate LoDTensor as its output.

Copy link
Member

@QiJune QiJune left a comment

Choose a reason for hiding this comment

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

LGTM

@reyoung reyoung merged commit f43b1a9 into PaddlePaddle:develop Oct 17, 2017
@reyoung reyoung deleted the feature/var_type_inferer branch October 28, 2017 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants