Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tensor layout #1237

Merged
merged 15 commits into from Sep 18, 2019
Merged

Tensor layout #1237

merged 15 commits into from Sep 18, 2019

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Sep 5, 2019

Why we need this PR?

  • It adds extensible string-like TensorLayout

What happened in this PR?

  • String-like TensorLayout object was added
  • Was this PR tested? How?
    • Unit tests
  • Were docs and examples updated, if necessary?
    • Doxygen

JIRA TASK: [DALI-525]

@mzient
Copy link
Contributor Author

mzient commented Sep 5, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [886996]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [886996]: BUILD PASSED

@mzient
Copy link
Contributor Author

mzient commented Sep 6, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888428]: BUILD STARTED

@mzient
Copy link
Contributor Author

mzient commented Sep 6, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888574]: BUILD STARTED

@mzient
Copy link
Contributor Author

mzient commented Sep 6, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888584]: BUILD STARTED

@mzient
Copy link
Contributor Author

mzient commented Sep 6, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888629]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888629]: BUILD FAILED

@mzient
Copy link
Contributor Author

mzient commented Sep 6, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888649]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [888649]: BUILD PASSED

Kh4L
Kh4L reviewed Sep 6, 2019
View reviewed changes
include/dali/core/tensor_layout.h Outdated
return NumSpatialDims(tl) == 2;
}
DALI_HOST_DEV
static int Is3D(const TensorLayout &tl) {
Copy link
Contributor

@Kh4L Kh4L Sep 6, 2019 •
edited

Choose a reason for hiding this comment

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

Why do these return an int while the latter three return a bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

@mzient mzient force-pushed the TensorLayout branch 3 times, most recently from 6f87a6f to d5e05ca Compare September 9, 2019 12:37
@mzient
Copy link
Contributor Author

mzient commented Sep 9, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [891677]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [891677]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [891778]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [891778]: BUILD FAILED

awolant
awolant reviewed Sep 9, 2019
View reviewed changes
include/dali/core/tensor_layout.h Show resolved Hide resolved
include/dali/core/tensor_layout.h Show resolved Hide resolved
include/dali/core/tensor_layout.h Show resolved Hide resolved
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [903893]: BUILD PASSED

szalpal
szalpal requested changes Sep 17, 2019
View reviewed changes
Copy link
Member

@szalpal szalpal left a comment

Choose a reason for hiding this comment

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

There are still a few of my comments, please respond to them

@szalpal
Copy link
Member

szalpal commented Sep 17, 2019

I'm wondering here, it might be my impression, but:
At some parts of this PR, it is assumed, that the interpretation of names/symbols of given layout components is left for the user, while at some others (e.g. in ImageInfo), it's standardized. I think it should be explicitly stated, when we expect given names/symbols and when it's left for the users interpretation

@mzient
Initial effort
43c94ea 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Add TensorLayout and accompanying functions for image and video layou…
64c82cf 
…t analysis.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Extended LayoutInfo.
8fe8392 
Added Doxygen comments.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Changed TensorLayout to fixed size.
cb31159 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Move TensorLayout to dali/core.
4331d01 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Add layout concatenation.
7adb894 
Add tests for concatenation and extraction.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Adjust TensorLayout for on-device usage.
5f5ba33 
Add device-side tests for TensorLayout.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix host/device qualifiers for nvcc 9.
cb95867 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Small string optimization with size/null aliasing.
7254bd7 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
* Add permutations.
26753a1 
* Use Alexandrescu's (?) trick to increase static capacity to 15.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Address review issues.
5fdf48e 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix review issues:
2ad8ab7 
- add comments
- add explicit tests for `skip` and `find`
- add test for `GetFrameLayout`

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Rename permuted_dims to GetLayoutMapping.
a450c1f 
Add explicit case operator to std::string.
Fix minor review issues.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Add more comments.
0b1cc04 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient mzient force-pushed the TensorLayout branch 2 times, most recently from aa5c6dc to a0cd9f6 Compare September 18, 2019 08:21
@mzient
Review stuff... don't get me started here.
9325c71 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Sep 18, 2019

!build

@mzient mzient requested a review from szalpal September 18, 2019 08:25
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [905487]: BUILD STARTED

szalpal
szalpal approved these changes Sep 18, 2019
View reviewed changes
Copy link
Member

@szalpal szalpal left a comment

Choose a reason for hiding this comment

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

Only minor thing left would be the indentatnions in switch...case statement, please fix if you would be so kind, so it matches the GoogleStyle

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [905487]: BUILD PASSED

@mzient mzient merged commit 196edf7 into NVIDIA:master Sep 18, 2019
00liujj pushed a commit to 00liujj/DALI that referenced this pull request Oct 10, 2019
@mzient @00liujj
Tensor layout (NVIDIA#1237)
6881521 
* Add TensorLayout and accompanying functions for image and video layout analysis.
* Add GetLayoutMapping function that finds fixed-size permutation of layouts.

TODO
* switch formatting
* maybe: add variable-sized GetLayoutMapping (with result in SmallVector)

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Jianjun Liu <00liujj@163.com>
@SomeoneSerge SomeoneSerge mentioned this pull request Mar 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kh4L Kh4L Kh4L left review comments

@awolant awolant awolant left review comments

@jantonguirao jantonguirao jantonguirao approved these changes

@szalpal szalpal szalpal approved these changes

@klecki klecki klecki approved these changes

@banasraf banasraf Awaiting requested review from banasraf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mzient @dali-automaton @jantonguirao @szalpal @Kh4L @klecki @awolant