LODTensor (Level of details, or Level of sequences Tensor). #3109

Superjomn · 2017-07-30T07:10:54Z

reyoung

Sorry for the late review.

reyoung · 2017-07-30T07:14:10Z

paddle/framework/tensor_test.cc

+class LODTensorTester : public ::testing::Test {
+ public:
+  virtual void SetUp() override {
+    lod_tensor = decltype(lod_tensor)(new LODTensor);


Maybe lod_tensor.reset(new LODTensor); could be simpler?

reyoung · 2017-07-30T07:15:04Z

paddle/framework/tensor.h

+  // Tesor.
+  typedef std::vector<level_t> lod_t;
+
+  explicit LODTensor() {}


explicit is not necessary.

reyoung · 2017-07-30T07:16:02Z

paddle/framework/tensor.h

+
+  explicit LODTensor() {}
+
+  LODTensor(LODTensor&& other)


Why should we move an LODTensor?

Maybe LODTensor(LODTensor&& ) = delete is enough?

to move shared_ptr<lod_t> and shared_ptr<Tensor>

reyoung · 2017-07-30T07:17:38Z

paddle/framework/tensor.h

+  /*
+   * Number of elements in a level.
+   */
+  size_t Elements(uint32_t level = 0) const {


Please use size_t not uint32_t for index.

Please always use size_t for index type because it is used widely in C++ standard library.

reyoung · 2017-07-30T07:19:01Z

paddle/framework/tensor.h

+
+  /*
+   * Slice of elements of a level, [elem_begin: elem_end]
+   * NOTE low performance in slice seq_start_positions_.


seq_start_positions_ --> lod_start_pos_

Also, we use Doxygen comment style. maybe

@note: Low performance in slice lod_start_pos_

is better.

reyoung · 2017-07-30T07:37:12Z

paddle/framework/tensor_test.cc

+
+    auto tensor = std::make_shared<Tensor>();
+    DDim dims = make_ddim({20 /*batch size*/, 128 /*dim*/});
+    tensor->Resize(dims);


tensor->Resize({20, 128})

is good.

reyoung · 2017-07-30T07:38:51Z

paddle/framework/tensor_test.cc

+    // 0 5 10 15 20
+    // 0 2 5 7 10 12 15 20
+    auto lod = std::make_shared<LODTensor::lod_t>();
+    lod->emplace_back(LODTensor::level_t{0, 10, 20});


auto lod = std::make_shared<LODTensor::lod_t>({ {0, 10, 20}, {0, 5, 10, 15, 20}, {0, 2, 5, 7, 10, 12, 15, 17, 20} });

I am not sure which is more readable, but C++ 11 make initialization of an std container very easy.

auto lod = std::make_shared<LODTensor::lod_t>({ {0, 2, 5, 7, 10, 12, 15, 17, 20} {0, 5, 10, 15, 20}, {0, 10, 20}, });

上次讨论的是，从小粒度开始存吧

lod->back() 。。还没写完的版本
我下午写完，再推上来 @luotao1

reyoung · 2017-07-30T07:43:02Z

paddle/framework/detail/tensor-inl.h

+  auto end = lod_start_pos_->at(level)[elem_end];
+  for (const auto& l : *lod_start_pos_) {
+    auto it_begin = std::find(l.begin(), l.end(), start);
+    auto it_end = std::find(it_begin, l.end(), end);


it_begin & it_end could be not found. Add a PADDLE_ENFORCE here.

reyoung · 2017-07-30T07:44:14Z

paddle/framework/detail/tensor-inl.h

+  auto new_tensor = tensor_->Slice<T>(start, end);
+
+  LODTensor res;
+  res.set_tensor(new_tensor);


Maybe set_tensor and set_lod should be constructor of LODTensor. Because all valid LODTensor should set them both.

reyoung · 2017-07-30T07:45:49Z

paddle/framework/detail/tensor-inl.h

@@ -138,5 +138,33 @@ inline void Tensor::Resize(const DDim& dims) { dims_ = dims; }

 inline const DDim& Tensor::dims() const { return dims_; }

+template <typename T>
+LODTensor LODTensor::Slice(uint32_t level, uint32_t elem_begin,


Add unit-test for Slice method to make sure that code is implemented right.

qingqing01 · 2017-08-01T13:38:08Z

paddle/framework/lod_tensor.h

+   * in the sentence's view, article, paragraph, sentence are 3 levels.
+   */
+  size_t Levels() const {
+    return lod_start_pos_.get() ? lod_start_pos_->size() : 0UL;


has_lod() ? lod_start_pos_->size() : 0UL;

qingqing01 · 2017-08-01T13:44:11Z

paddle/framework/lod_tensor.cc

+  auto start = lod.at(level)[elem_begin];
+  auto end = lod.at(level)[elem_end];
+
+  LOG(INFO) << "start: " << start << " end: " << end;


remove this line.

qingqing01 · 2017-08-01T14:07:15Z

paddle/framework/lod_tensor.h

+
+std::shared_ptr<LODTensor::lod_t> SliceLOD(const LODTensor::lod_t &lod,
+                                           size_t level, size_t elem_begin,
+                                           size_t elem_end);


Add unit test for these two SliceLOD interfaces. And these interfaces need comments. Some examples like:

lod={{0, 3, 7, 9}} ， level=0， elem_begin=1， elem_end=2, return new_load = {{3，7}}

It is useful for understanding to other developers.

qingqing01 · 2017-08-01T14:16:05Z

paddle/framework/lod_tensor.h

+  auto new_tensor = std::make_shared<Tensor>();
+  new_tensor->template CopyFrom<T>(sliced_tensor, dst_place);
+
+  return LODTensor(new_tensor, new_lod);


lod_start_pos_的slice从上面的实现来看，没有对index减去起始值。比如3条句子：

a b c e f g h j k

lod={{0, 3, 7, 9}} ，
level=0， elem_begin=1， elem_end=2,
slice之后 new_lod = {{3，7}}，而不是 {{0,4}}

tensor_进行了Slice，并且拷贝。

这样，新的LODTensor依据new_lod来访问，会有越界吧

qingqing01 · 2017-08-01T14:19:53Z

paddle/framework/lod_tensor.h

+  auto new_tensor = std::make_shared<Tensor>();
+  new_tensor->CopyFrom<T>(*tensor_, dst_place);
+
+  return LODTensor(new_tensor, new_lod);


level级别的slice，这里对lod_start_pos_进行了slice，tensor_没有slice，是完整拷贝？

觉得最好多加一些注释，或者例子解释，不然developers看到还得思考半天~

reyoung · 2017-08-03T06:02:28Z

paddle/framework/lod_tensor.h

+  typedef std::vector<level_t> lod_t;
+
+  LODTensor() {}
+  LODTensor(std::shared_ptr<Tensor> tensor, std::shared_ptr<lod_t> lod) {


If use shared_ptr, always use const std::shared_ptr<Tensor>& to pass them into a function.
std::shared_ptr<T> could be very slow.

Here need to share resource, so no const& here.

Here need to share resource, so no const& here.

const std::shared_ptr<T>& does not mean that the value of T cannot be changed. It means that the pointer of T cannot be changed.

reyoung · 2017-08-03T06:02:40Z

paddle/framework/lod_tensor.h

+    Reset(tensor, lod);
+  }
+
+  void Reset(std::shared_ptr<Tensor> tensor, std::shared_ptr<lod_t> lod) {


Same as above.

reyoung · 2017-08-03T06:05:36Z

paddle/framework/lod_tensor.h

+    PADDLE_ENFORCE(elem < Elements(level),
+                   "element begin [%d] out of range [%d]", elem,
+                   Elements(level));
+    return lod_start_pos_->at(level)[elem];


lod_start_pos_[level][elem]; is better, because the previous PADDLE_ENFORCE has check the index of lod_start_pos_, it is not necessary use at to check it again.

The different between at and [] is at will perform a range check, and throw a std::runtime_error when out of range.

reyoung · 2017-08-03T06:09:25Z

paddle/framework/lod_tensor.h

+  /*
+   * Number of elements in a level.
+   */
+  size_t Elements(size_t level = 0) const {


Levels and Elements are quite confusing names. Maybe just simple NumOfLevels and NumOfElements are better name? Or NumLevels?

See https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

num_errors is a good name.

reyoung · 2017-08-03T06:14:05Z

paddle/framework/lod_tensor.h

+  /*
+   * Determine whether LODTensor has a valid LOD info.
+   */
+  bool has_lod() const { return lod_start_pos_.get(); }


return lod_start_pos_;

reyoung · 2017-08-03T06:16:52Z

paddle/framework/lod_tensor.h

+   * Determine whether LODTensor has a valid LOD info.
+   */
+  bool has_lod() const { return lod_start_pos_.get(); }
+  std::shared_ptr<lod_t> const lod() const { return lod_start_pos_; }


http://www.cplusplus.com/reference/memory/const_pointer_cast/

std::shared_ptr<const lod_t> lod() const { return std::const_pointer_cast<const lod_t>(lod_start_pos_)}

reyoung · 2017-08-03T06:18:16Z

paddle/framework/lod_tensor.h

+
+ private:
+  mutable std::shared_ptr<lod_t> lod_start_pos_;
+  mutable std::shared_ptr<Tensor> tensor_;


Do not use mutable.

reyoung · 2017-08-03T06:22:51Z

paddle/framework/lod_tensor.cc

+  return LODTensor(tensor_, new_lod);
+}
+
+namespace details {


The details namespace is not necessarily declared in the header file.
Just declare them in *.cc is simpler.

wangkuiyi · 2017-08-03T06:20:54Z

paddle/framework/lod_tensor.h

+  // LOD stores offsets of each level of units, the largest units level first,
+  // then the smaller units level. Each level_t stores the offsets of units in
+  // Tesor.
+  typedef std::vector<level_t> lod_t;


lod_t => LOD

Following Google C++ style guide: https://google.github.io/styleguide/cppguide.html#Type_Names

wangkuiyi · 2017-08-03T06:21:25Z

paddle/framework/lod_tensor.h

+  /*
+   * Determine whether LODTensor has a valid LOD info.
+   */
+  bool has_lod() const { return lod_start_pos_.get(); }


has_lod => HasLOD

Follow Google C++ style guide https://google.github.io/styleguide/cppguide.html#Function_Names

wangkuiyi · 2017-08-03T06:25:03Z

paddle/framework/lod_tensor.h

+  mutable std::shared_ptr<Tensor> tensor_;
+};
+
+namespace details {


According to C++ style https://google.github.io/styleguide/cppguide.html#Namespace_Names, if we want to add namespace details, we must create a corresponding sub-directory named details.

Here it seems that we don't need this namespace nor the sub-directory -- if SliceLOD is supposed to be called outside of framework/lod_tensor*, let's just remove namespace details; otherwise, let's move it into lod_tensor.cc and in the anonymous namespace as described in https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

wangkuiyi · 2017-08-03T06:26:04Z

paddle/framework/lod_tensor.h

+   * Slice of levels[level_begin:level_end], with tensor copied.
+   */
+  template <typename T>
+  LODTensor SliceCopied(size_t level_begin, size_t level_end,


T doesn't appear in the definition of SliceCopied. Do we really need this method a function template?

I have the same question with SliceShared.

tensor's CopyFrom need a T, SliceShared's T will be removed.

wangkuiyi · 2017-08-03T06:27:02Z

paddle/framework/tensor.cc

@@ -15,5 +15,5 @@
 #include "paddle/framework/tensor.h"

 namespace paddle {
-namespace framework {}
+namespace framework {}  // namespace framework


Why would we need this empty .cc file?

reyoung · 2017-08-03T06:23:27Z

paddle/framework/lod_tensor.cc

+std::shared_ptr<LODTensor::lod_t> SliceLOD(const LODTensor::lod_t &lod,
+                                           size_t level_begin,
+                                           size_t level_end) {
+  auto new_lod = std::make_shared<LODTensor::lod_t>();


new_lod.reserve.

Always reserve vector.

reyoung · 2017-08-03T06:24:09Z

paddle/framework/lod_tensor.cc

+                                           bool tensor_shared) {
+  // slice the lod.
+  auto new_lod = std::make_shared<LODTensor::lod_t>();
+  auto start = lod.at(level)[elem_begin];


is at necessary?

reyoung · 2017-08-03T06:25:01Z

paddle/framework/lod_tensor.cc

+                                           size_t elem_end,
+                                           bool tensor_shared) {
+  // slice the lod.
+  auto new_lod = std::make_shared<LODTensor::lod_t>();


reserve if you can.

reyoung · 2017-08-03T06:28:23Z

paddle/framework/lod_tensor.h

+ * @level_begin: level to begin slice.
+ * @level_end: level to end slice.
+ */
+std::shared_ptr<LODTensor::lod_t> SliceLOD(const LODTensor::lod_t &lod,


namespace details is not necessarily in the header file.

reyoung · 2017-08-03T06:29:53Z

paddle/framework/lod_tensor_test.cc

+
+TEST_F(LODTensorTester, SliceShared_Level) {
+  // slice 1 level
+  for (int level = 0; level < 3; level++) {


always use size_t for loop index. always use ++level.

https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

wangkuiyi

LODTensor is a Tensor, so it should be derived from class Tensor, so could it inherit all methods from Tensor.

LOD type should be exported in lod.{h,cc,_test.cc}, so that it could be assigned to a Variable instance.

In our applications, many LODTensor instances might share the same LOD instance. We can save the LOD instance in a variable in the global scope, so that it would be referred to by all LODTensor instances.

The constructor of an LODTensor should be

class LODTensor {
 public:
  LODTensor(const LOD& lod, ...) : lod_(lod), Tensor(...) {}

LODTensor doesn't need SliceCopied. Let us remvoe it.

LODTensor needs SliceLevels and SliceInLevel.

Variable type inference along the network should be implemented using simple C++ features. Consider that an RNNOp instance would require its inputs are of type LODTensor, and should return outputs in LODTensor too. An FCOp instance requires Tensor-typed inputs and would return Tensor-typed outputs. However, it is possible that an LODTensor input feeds into an RNNOp instance, the output feeds into a FCOp instance, and the output of FCOp instance feeds in a second RNNOp instance. In such case, we want the FCOp instance creates its outputs in type LODTensor. To implement this features, we need to:

Add virtual Tensor* Tensor::Close() const

Override it in LODTensor as

class LODTensor {
 public:
  virtual Tensor* Clone() const {
    return new LODTensor(lod_);
  }

In RNNOp and FCOp, the creation of output must be cloned from the input. For example:
```
Tensor* output = Input(0)->Clone();
```

wangkuiyi · 2017-08-09T01:06:56Z

paddle/framework/lod_tensor.cc

+
+LODTensor LODTensor::SliceShared(size_t level, size_t elem_begin,
+                                 size_t elem_end) const {
+  PADDLE_ENFORCE(HasLOD(), "has no LOD info, can't be sliced.");


一个LODTensor instance里不是一定总有LOD 信息的？

Tensor* output = Input(0)->Clone();

这里是否应该改成

output_var.clone(input_var); // clone the tensor type Tensor* output = output_var.Get<Tensor>();

@wangkuiyi

wangkuiyi · 2017-08-09T01:08:16Z

paddle/framework/lod_tensor.h

+  /*
+   * Get a element from LOD.
+   */
+  size_t lod_element(size_t level, size_t elem) const {


lod_element => StartPosition

wangkuiyi · 2017-08-09T01:08:40Z

paddle/framework/lod_tensor.h

+  /*
+   * Number of elements in a level.
+   */
+  size_t NumElements(size_t level = 0) const {


NumElements => ElementsOfLevel

wangkuiyi · 2017-08-09T01:09:33Z

paddle/framework/lod_tensor.h

+  }
+
+  /*
+   * Slice of levels[level_begin:level_end], with tensor copied.


// Copy the slice of data from *this into a new LODTensor and return the new LODTensor.

wangkuiyi · 2017-08-09T01:10:16Z

paddle/framework/lod_tensor.h

+  /*
+   * Slice of levels[level_begin:level_end], with tensor shared.
+   */
+  LODTensor SliceShared(size_t level_begin, size_t level_end) const;


// Returns a new LODTensor that shares data with *this but provides a view of the specified slice.

wangkuiyi · 2017-08-09T01:14:24Z

paddle/framework/lod_tensor.h

+   * Slice of levels[level_begin:level_end], with tensor copied.
+   */
+  template <typename T>
+  LODTensor SliceCopied(size_t level_begin, size_t level_end,


Does this method slice some levels, or elements in a level?

wangkuiyi · 2017-08-09T01:14:52Z

paddle/framework/lod_tensor.h

+  Tensor *raw_tensor() { return tensor_.get(); }
+
+ private:
+  std::shared_ptr<LOD> lod_start_pos_;


lod_start_pos_ = > lod_

Superjomn added 5 commits July 29, 2017 15:44

init lottensor

0e91e08

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

0ba4f1d

Merge branch 'fix_rnn' into lottensor

246a83a

init lottensor

32c948a

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

f2c2a64

reyoung reviewed Jul 30, 2017

View reviewed changes

Superjomn mentioned this pull request Jul 30, 2017

tensor type deduce #3110

Closed

Superjomn changed the title ~~LOTTensor~~ LODTensor (Level of details, or Level of sequences Tensor). Jul 31, 2017

Superjomn added 5 commits July 31, 2017 21:37

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

d9c4074

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

54f4749

finish lodtensor

660fc06

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

4dcc825

add lodtensor

a1342c8

qingqing01 reviewed Aug 1, 2017

View reviewed changes

Superjomn added 3 commits August 2, 2017 17:23

add reshape of lod

c4e751a

recover .clang-format

6a0953c

fix test lod

76a9c8b

reyoung reviewed Aug 3, 2017

View reviewed changes

wangkuiyi requested changes Aug 3, 2017

View reviewed changes

reyoung reviewed Aug 3, 2017

View reviewed changes

Superjomn added 8 commits August 3, 2017 15:48

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

018f121

add details

9cd8033

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

62cab87

rename Elements/Levels

71108f8

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

0607a28

size_t and vector reserve

8e970b3

add details

df75fe2

add const& std::shared_ptr

14858cd

Superjomn added 8 commits August 5, 2017 17:31

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

1c64657

add lod_tensor_impl.h

9319c1e

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

9c84313

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

52adce2

fix compile error

adfe84d

fix thrust vector

180499c

remove a shared_ptr

98a3de5

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into lottensor

710d863

wangkuiyi approved these changes Aug 9, 2017

View reviewed changes

Superjomn merged commit ede02d7 into PaddlePaddle:develop Aug 9, 2017

Superjomn deleted the lottensor branch August 9, 2017 01:05

wangkuiyi reviewed Aug 9, 2017

View reviewed changes

wangkuiyi mentioned this pull request Aug 9, 2017

Improve LODTensor #3345

Closed

LODTensor (Level of details, or Level of sequences Tensor). #3109

LODTensor (Level of details, or Level of sequences Tensor). #3109

Conversation

Superjomn commented Jul 30, 2017 • edited Loading

reyoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Superjomn Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luotao1 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Superjomn Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qingqing01 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

qingqing01 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Superjomn commented Jul 30, 2017 •

edited

Loading

Superjomn Jul 31, 2017 •

edited

Loading

luotao1 Aug 1, 2017 •

edited

Loading

Superjomn Aug 1, 2017 •

edited

Loading

qingqing01 Aug 1, 2017 •

edited

Loading

qingqing01 Aug 1, 2017 •

edited

Loading

reyoung Aug 3, 2017 •

edited

Loading