-
Notifications
You must be signed in to change notification settings - Fork 609
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.
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
Add supported layouts to Crop, CropMirrorNormalize #3722
Conversation
dali/pipeline/data/layouts.h
Outdated
// Layouts containing spatial dimensions such as height, width, depth | ||
// Contains Image, Volumetric Images, and Sequence layouts | ||
static std::initializer_list<TensorLayout> kSpatialLayouts { | ||
"HWC", "CHW", "DHWC", "FHWC", "FCHW", "CDHW", "CFHW", "FDHWC", "FCDHW", "CFDHW" |
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.
These are not spatial layouts, since they also include temporal dimension.
Also, I don't like mixing such lists into a high-impact (and therefore, high build-time) header.
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.
Removed it
73bafce
to
fdc5c39
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.
Should we have some tests for not compatible input layouts or is it overkill?
Signed-off-by: Joaquin Anton <janton@nvidia.com>
5f4d19c
to
bd49f8d
Compare
Signed-off-by: Joaquin Anton <janton@nvidia.com>
.InputLayout(0, {"HWC", "CHW", "DHWC", | ||
"FHWC", "FCHW", "CDHW", "CFHW", |
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.
Nitpick: I know it technically only matters which layout is listed first for any given number of dimensions, but it would be nice to group them as 3D, video (or, conversely: video, 3D).
.InputLayout(0, {"HWC", "CHW", "DHWC", | |
"FHWC", "FCHW", "CDHW", "CFHW", | |
.InputLayout(0, {"HWC", "CHW", "DHWC", "CDHW", | |
"FHWC", "FCHW","CFHW", |
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
!build |
CI MESSAGE: [4116328]: BUILD STARTED |
CI MESSAGE: [4116328]: BUILD PASSED |
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton janton@nvidia.com
Category:
New feature
Description:
Adds layout information to Crop, CropMirrorNormalize. The layout info is used both for documentation and error checking.
Additional information:
Affected modules and functionalities:
Crop, CropMirrorNormalize operators
Key points relevant for the review:
NA
Checklist
Tests
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A