Skip to content

Comments

Modify Pybind LoDTensor API according to length-based LoD#11106

Merged
kexinzhao merged 16 commits intoPaddlePaddle:developfrom
kexinzhao:fix_lod_pybind
Jun 15, 2018
Merged

Modify Pybind LoDTensor API according to length-based LoD#11106
kexinzhao merged 16 commits intoPaddlePaddle:developfrom
kexinzhao:fix_lod_pybind

Conversation

@kexinzhao
Copy link
Contributor

@kexinzhao kexinzhao commented Jun 1, 2018

Some background can be found here.

Essentially, our level of detail info (LoD) is currently both implemented in C++ and exposed in Python using "offsets". We want to encapsulate the offset implementation detail in C++ and only expose length based interface in Python.

What this PR does (See the main change in pybind.cc here):

  1. Previously, the set_lod and lod API in pybind.cc uses offset_based LoD.
    This PR modifies the LoDTensor API in pybind.cc so that the user on the python side can provide or obtain length-based LoD using set_lod and lod, respectively.

  2. The init API is also modified so that user can create a LoDTensor object in Python and initialize it with a length-based LoD.

  3. A has_valid_lod API is added in pybind.cc so that user can check if the LoD info in a tensor is valid and matches the its data dimension on the python side.

  4. Utility functions to create LoDTensor in the python side defined here is largely simplified using the updated set_lod and lod in pybind. (See changes here)

  5. Over 30 tests that uses set_lod and lod has been modified to reflect the updated LoDTensor API in pybind.cc.

@kexinzhao kexinzhao force-pushed the fix_lod_pybind branch 30 times, most recently from 077235f to ae366a8 Compare June 4, 2018 18:01
@kexinzhao kexinzhao force-pushed the fix_lod_pybind branch 3 times, most recently from 77fd3d5 to 75816da Compare June 14, 2018 23:53
panyx0718
panyx0718 previously approved these changes Jun 15, 2018
Copy link
Contributor

@panyx0718 panyx0718 left a comment

Choose a reason for hiding this comment

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

Looks Good

std::back_inserter(new_lod));
LoD new_offset_lod = ConvertToOffsetBasedLoD(new_lod);
PADDLE_ENFORCE(CheckLoD(new_offset_lod, -1),
"the provided lod info is invalid");
Copy link
Contributor

Choose a reason for hiding this comment

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

lod to reursive_sequence_lengths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@panyx0718
Copy link
Contributor

Those failed tests. I'm wondering if the test can catch the exceptions.

@panyx0718 panyx0718 closed this Jun 15, 2018
@panyx0718 panyx0718 reopened this Jun 15, 2018
@kexinzhao kexinzhao force-pushed the fix_lod_pybind branch 5 times, most recently from 61cc22a to d7fb929 Compare June 15, 2018 03:45
@kexinzhao
Copy link
Contributor Author

kexinzhao commented Jun 15, 2018

@panyx0718 I have catch the exceptions in the test_lod_tensor. For other failed op test, because they use the optest module, so it is very difficult to catch the exception. So instead, I simply fix the invalid lods. Now it passes all the tests. Please take another look when you have time.

@kexinzhao kexinzhao merged commit 417fcf4 into PaddlePaddle:develop Jun 15, 2018
@kexinzhao kexinzhao deleted the fix_lod_pybind branch June 15, 2018 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants