-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
BeamSearchDecodeOp #5498
BeamSearchDecodeOp #5498
Conversation
paddle/operators/trieconcat_op.h
Outdated
@@ -0,0 +1,248 @@ | |||
/* Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operator does not need header file at all. You can put all code into .cc file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a header file is easier for testing. because this op is a little complex, it need to be tested carefully in CPP.
paddle/operators/trieconcat_op.h
Outdated
father->kids_.push_back(this); | ||
} | ||
|
||
BeamNode* father_ = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
father --> parent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
paddle/operators/trieconcat_op.h
Outdated
const size_t kSentenceLevel = 1; | ||
|
||
struct BeamNode { | ||
BeamNode(int64_t word_id, float prob) : word_id_(word_id), prob_(prob) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a destructor here
~BeamNode() {
if (parent_) {
parent_->DropKid(this);
if (parent_->NumKid() == 0) {
delete parent_;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, I use a function RemoveFromEnd to do this, but it need to be optimized~ thank you for your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
paddle/operators/trieconcat_op.h
Outdated
namespace paddle { | ||
namespace operators { | ||
|
||
using value_type = float; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO NOT ASSUME THE VALUE TYPE IS FLOAT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a problem to be fixed after the main process is done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, use a template to describe score.
using LoDTensor = framework::LoDTensor; | ||
using LoDTensorArray = framework::LoDTensorArray; | ||
|
||
const int64_t kEndId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a customizable attribute that can be modified by users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kEndId
is removed, the user can handle the result candidate in Python.
EXPECT_EQ(lod[0], expect_source_lod); | ||
std::vector<size_t> expect_sentence_lod = {0, 2, 5, 8, 11, 14, 17}; | ||
EXPECT_EQ(lod[1], expect_sentence_lod); | ||
std::vector<int> expect_data = {1, 0, 3, 1, 0, 3, 2, 1, 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the prefixes without ending are dropped.
This is not the ideal case, the prefixes should be stored so that user can make use of the detailed result.
The data should be
1,0,2,3,1,0,3,2,1,4,3,2,4,4,3,5,6,5,4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after discuss with @Superjom and @lcy-seso , we decide to keep all the possible result sentence.
} | ||
VLOG(3) << "this sentence has no more candidate, prune it"; | ||
// remove this sentence from Beam Tree. | ||
delete prefix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be replaced with a unque_ptr, that is much safer to operate on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fix: #5597
project: #5333
BeamSearch op will output a LodTensorArray, each element is the candidate word id and score of one step. This operator is used to concat the word ids of each steps into one LodTensor of candidate sentences(id/score).