-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
8c4a734
to
c36ac6f
Compare
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.
One main comment. Great job!
src/operator/sequence_mask.cc
Outdated
) | ||
.describe(R"code(Sets all elements outside the sequence to a constant value. | ||
|
||
This function takes an n-dimensional input array of the form [max sequence length, batch size, other dims] |
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.
maybe better to indicate some how that "max sequence length" refers to a single quantity (it does, correct?). I suggest just using underscores:
[max_sequence_length, batch_size, other dims]
Now for "other dims" -- what does that mean? it's not obvious form the context.
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.
Used underscore for parameters. other_dims here means extra feature dimensions which doesn't matter for these sequence* functions. I changed it to other_feature_dims now. Is it fine?
* sort,argsort,topk,argmin,argmax,argmax_channel docs modified * some sentence structure changes * revert some changes made * minor change * sentence restructering * sequencelast modified * minor word changes * formatting fixes * SequenceMask added * docs for SequenceReverse,true_divide is added * line break * minor change * take, batch_take operators added * minor formatting changes * Changes after review * note modified
Modified docs for functions sort, argsort, argmin, argmax, argmax_channel, sequenceLast, SequenceMask, SequenceReverse, true_divide, take, batch_take.
@mli, @zackchase, @nswamy @madjam @indhub @jiajiechen Please take a look.