Lookup table support selected rows as parameter #9575
Conversation
jacquesqiao
requested review from
chengduoZH,
Yancey1989,
typhoonzero and
qingqing01
… lookup_table_support_SelectedRows_as_parameter
| auto *table_t = context.Input<SelectedRows>("W"); | ||
| table_dim = table_t->value().dims(); | ||
| } else { | ||
| PADDLE_THROW("table only support LoDTensor and SelectedRows"); |
There was a problem hiding this comment.
table => LookupTable
support => supports
| framework::ToDataType(table_var->Get<SelectedRows>().value().type()), | ||
| ctx.device_context()); | ||
| } else { | ||
| PADDLE_THROW("W should be LoDTensor or SelectedRows"); |
There was a problem hiding this comment.
The parameter W of a LookupTable must be either LoDTensor or SelectedRows
| @@ -25,56 +28,88 @@ namespace operators { | |||
| using Tensor = framework::Tensor; | |||
| using LoDTensor = framework::LoDTensor; | |||
| using SelectedRows = framework::SelectedRows; | |||
| using DDim = framework::DDim; | |||
|
|
|||
| static constexpr int64_t kNoPadding = -1; | |||
There was a problem hiding this comment.
static means that this variable is not exposed and is often used in .cc files. Why do we need static in this header file?
|
|
||
| static constexpr int64_t kNoPadding = -1; | ||
|
|
||
| inline size_t getIndex(const std::vector<int64_t> &rows, int64_t value) { |
There was a problem hiding this comment.
getIndex is not style-compatible. Should be GetIndex.
Also, GetIndex is a too general name; we might need a specific name, or move it into a class, which represents the context of the name.
There was a problem hiding this comment.
Ok, will move it to selected_rows.cc
| auto *table_t = context.Input<SelectedRows>("W"); | ||
| table_dim = table_t->value().dims(); | ||
| } else { | ||
| PADDLE_THROW("table only support LoDTensor and SelectedRows"); |
There was a problem hiding this comment.
The parameter W of LookupTable must be either LoDTensor or SelectedRows.
fix: #9576
task_list: #9211
project: https://github.com/PaddlePaddle/Paddle/projects/56