[Cpp API Compatibility] add layout API #77185
Conversation
|
你的PR提交成功，感谢你对开源项目的贡献! |
There was a problem hiding this comment.
Pull request overview
This PR adds a layout() API to the TensorBase class for C++ API compatibility with PyTorch/ATen. The implementation maps Paddle's DataLayout enum values to corresponding c10::Layout values to maintain API consistency.
Key Changes:
- Added
layout()method toTensorBaseclass that converts Paddle'sDataLayouttoc10::Layout - Added necessary include for
c10/core/Layout.hin both implementation and test files - Added unit test to verify the layout API works correctly for strided tensors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
paddle/phi/api/include/compat/ATen/core/TensorBase.h |
Implements the layout() method with a switch statement mapping various DataLayout types (STRIDED, NCHW, NHWC, etc.) to corresponding c10::Layout values (kStrided, kSparse, kSparseCsr, kMkldnn) |
test/cpp/compat/compat_basic_test.cc |
Adds test case LayoutAPI that verifies default tensors return kStrided layout and tests the layout output stream operator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (tensor_.layout()) { | ||
| case common::DataLayout::STRIDED: | ||
| case common::DataLayout::NCHW: | ||
| case common::DataLayout::NHWC: | ||
| case common::DataLayout::NCDHW: | ||
| case common::DataLayout::NDHWC: | ||
| return c10::kStrided; | ||
| case common::DataLayout::SPARSE_COO: | ||
| return c10::kSparse; | ||
| case common::DataLayout::SPARSE_CSR: | ||
| return c10::kSparseCsr; | ||
| case common::DataLayout::ONEDNN: | ||
| return c10::kMkldnn; | ||
| default: | ||
| return c10::kStrided; | ||
| } |
There was a problem hiding this comment.
The switch statement is missing a case for common::DataLayout::PSTRING_UNION. According to the paddle/common/layout.h file, PSTRING_UNION is a valid DataLayout enum value. Without an explicit case for it, it will fall through to the default case and return c10::kStrided, which may not be the correct mapping. Consider adding an explicit case for PSTRING_UNION or documenting why it should default to kStrided.
| case common::DataLayout::SPARSE_CSR: | ||
| return c10::kSparseCsr; | ||
| case common::DataLayout::ONEDNN: | ||
| return c10::kMkldnn; |
There was a problem hiding this comment.
The switch statement is missing cases for common::DataLayout::UNDEFINED and common::DataLayout::ANY. According to the paddle/common/layout.h file, these are valid DataLayout enum values (with ANY being an alias for UNDEFINED). While they fall through to the default case and return c10::kStrided, it would be clearer to explicitly handle these cases or document the intended behavior for undefined/uninitialized layouts.
| return c10::kMkldnn; | |
| return c10::kMkldnn; | |
| case common::DataLayout::UNDEFINED: | |
| case common::DataLayout::ANY: | |
| // Treat undefined/any Paddle layouts as strided in ATen. | |
| return c10::kStrided; |
| TEST(TensorBaseTest, LayoutAPI) { | ||
| // Test layout() API for strided tensors | ||
| at::TensorBase tensor = at::ones({2, 3}, at::kFloat); | ||
|
|
||
| // Default tensor should have Strided layout | ||
| ASSERT_EQ(tensor.layout(), c10::kStrided); | ||
|
|
||
| // Test layout output stream operator | ||
| std::ostringstream oss; | ||
| oss << tensor.layout(); | ||
| ASSERT_EQ(oss.str(), "Strided"); | ||
| } |
There was a problem hiding this comment.
The test only covers the strided layout case. Consider adding test cases for other layout types that are supported by the layout() method, such as SPARSE_COO (maps to c10::kSparse), SPARSE_CSR (maps to c10::kSparseCsr), and ONEDNN (maps to c10::kMkldnn). This would ensure the switch statement logic in the layout() method is properly validated for all supported layout types.
| Layout layout() const { | ||
| switch (tensor_.layout()) { | ||
| case common::DataLayout::STRIDED: | ||
| case common::DataLayout::NCHW: | ||
| case common::DataLayout::NHWC: | ||
| case common::DataLayout::NCDHW: | ||
| case common::DataLayout::NDHWC: | ||
| return c10::kStrided; | ||
| case common::DataLayout::SPARSE_COO: | ||
| return c10::kSparse; | ||
| case common::DataLayout::SPARSE_CSR: | ||
| return c10::kSparseCsr; | ||
| case common::DataLayout::ONEDNN: | ||
| return c10::kMkldnn; | ||
| default: | ||
| return c10::kStrided; | ||
| } | ||
| } |
There was a problem hiding this comment.
The layout() method lacks documentation. Consider adding a comment explaining what the method returns and how Paddle's DataLayout values map to c10::Layout values. This would help developers understand the mapping logic, especially for edge cases like PSTRING_UNION that fall through to the default case.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #77185 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 1
Lines ? 2
Branches ? 0
===========================================
Hits ? 2
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/re-run all-failed |
1 similar comment
|
/re-run all-failed |
|
/re-run all-failed |
|
/re-run all-failed |
5 participants
PR Category
Environment Adaptation
PR Types
New features
Description
新增
layout兼容接口