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

CropMirrorNormalize full pad support #2044

Merged
merged 25 commits into from
Jul 1, 2020
Merged

CropMirrorNormalize full pad support #2044

merged 25 commits into from
Jul 1, 2020

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented Jun 22, 2020 •
edited
Loading

Why we need this PR?

Pick one, remove the rest

  • Refactoring SliceFlipNormalizePadPermute kernels and CropMirrorNormalize operator to support slicing out of bounds (same as it was done for Slice/Crop)

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    Rewrote SliceFlipNormalizePadPermute CPU and GPU kernels to support padding
    Adjusted CropMirrorNormalize to use the new kernels
  • Affected modules and functionalities:
    CropMirrorNormalize CPU and GPU operators, SliceFlipNormalizePadPermute CPU and GPU kernels
  • Key points relevant for the review:
    Kernels implementation
  • Validation and testing:
    New tests added
  • Documentation (including examples):
    N/A

JIRA TASK: [DALI-1462], [DALI-1464]

@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1417255]: BUILD STARTED

@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1417381]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1417381]: BUILD FAILED

@jantonguirao jantonguirao force-pushed the cmn_pad_support branch 6 times, most recently from 9a53545 to 014832a Compare June 25, 2020 11:07
@jantonguirao jantonguirao changed the title [WIP][DO NOT REVIEW] CropMirrorNormalize full pad support CropMirrorNormalize full pad support Jun 25, 2020
@jantonguirao jantonguirao requested a review from a team June 25, 2020 13:43
@jantonguirao
WIP
8a09822 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
WIP part 2
bc79894 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
mzient
mzient reviewed Jun 29, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_cpu.h Outdated
Comment on lines 224 to 227
VALUE_SWITCH(need_normalize ? 1 : 0, NeedNormalizeInt, (0, 1), (
VALUE_SWITCH(has_channels ? 1 : 0, HasChannelsInt, (0, 1), (
constexpr bool NeedNormalize = static_cast<bool>(NeedNormalizeInt);
constexpr bool HasChannels = static_cast<bool>(HasChannelsInt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Meh...

JanuszL
JanuszL reviewed Jun 29, 2020
View reviewed changes
dali/test/python/test_operator_crop_mirror_normalize.py Outdated
for out_of_bounds_policy in ['pad', 'trim_to_shape']:
for device in ['gpu', 'cpu']:
for batch_size in [1, 3]:
for out_layout in ["HWC", "CHW"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for out_layout in ["HWC", "CHW"]:
for out_layout in ['HWC', 'CHW']:

To be consistent with other test parameters.

JanuszL
JanuszL reviewed Jun 29, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_cpu.h
if (pad_before > 0) {
if (HasChannels && d == channel_dim) {
for (int64_t i = 0; i < pad_before; i++)
*output++ = *fill_values++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add {} around this line as it is easy to forget that this loop executes only this line.

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

JanuszL
JanuszL reviewed Jun 29, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_gpu.h Outdated
sample_desc.need_flip |= processed_args.in_strides[d] < 0;
need_flip |= sample_desc.need_flip;

// We the last dimension with the previous if:
Copy link
Contributor

Choose a reason for hiding this comment

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

We < what > last...?

JanuszL
JanuszL reviewed Jun 29, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_gpu.h Outdated

norm_mul_gpu = context.scratchpad->ToGPU(
context.gpu.stream, make_span(norm_mul_cpu, num_samples * norm_args_size_));
CUDA_CALL(cudaGetLastError());
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is enough to check this only once for the whole kernel run?

@jantonguirao
Fix tests
5b83017 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Fixes
b98a22a 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1431307]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1431307]: BUILD FAILED

@jantonguirao
Fix
61f17ec 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1433732]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1433732]: BUILD PASSED

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_common.h Outdated
Comment on lines 64 to 66
std::vector<float> mean;
std::vector<float> inv_stddev;
int normalization_dim;
float padding_val = 0.0f;
std::vector<float> fill_values;
Copy link
Contributor

@mzient mzient Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can use SmallVector<float, 4> here (not sure about un-processed args).

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_gpu.h Outdated
}
sample_desc.effective_ndim = last_dim + 1;

sample_sizes[i] = volume(processed_args.out_shape);
Copy link
Contributor

@mzient mzient Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

sample_size vector is not really necessary - it's elements are written once and then read once in the same order - you could calculate the volume directly

size_t remaining = volume(processed_args_[i].out_shape);

(GitHub won't let me put this comment on that line :()

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/operators/image/crop/crop_mirror_normalize.h Outdated
}

if (pad_channels) {
nchannels = NextPowerOfTwo(nchannels); // modifies args.shape
Copy link
Contributor

Choose a reason for hiding this comment

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

use next_pow2 from core/util.h and remove the redundant implementation.

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/test/python/test_operator_crop_mirror_normalize.py

def check_cmn_with_out_of_bounds_policy_support(device, batch_size, dtype, input_layout, input_shape, output_layout,
mirror_probability, mean, std, should_pad,
out_of_bounds_policy=None, fill_values=(0x76, 0xb9, 0x00)):
Copy link
Contributor

Choose a reason for hiding this comment

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

💚

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/test/python/test_operator_crop_mirror_normalize.py Outdated
Comment on lines 299 to 311
dtype = dtype,
output_layout = output_layout,
crop_d = crop_d,
crop_h = crop_h,
crop_w = crop_w,
crop_pos_x = crop_pos_x,
crop_pos_y = crop_pos_y,
crop_pos_z = crop_pos_z,
mean = mean,
std = std,
pad_output = pad_output,
out_of_bounds_policy = out_of_bounds_policy,
fill_values = fill_values)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: weird indent

mzient
mzient reviewed Jul 1, 2020
View reviewed changes
dali/kernels/slice/slice_flip_normalize_permute_pad_gpu.h
Comment on lines +248 to +249
VALUE_SWITCH(need_pad ? 1 : 0, NeedPad, (false, true), (
VALUE_SWITCH(need_flip ? 1 : 0, NeedFlip, (false, true), (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: you can put a comment about this conversion to int like:
// Convert switch argument to `int` to avoid compiler warning about unreachable case label
or similar - otherwise someone might try to "fix" it.

@jantonguirao
Code review fixes
6ac515c 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Code review fixes and cleanup
9ea4f86 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1437164]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1437164]: BUILD PASSED

@jantonguirao jantonguirao merged commit 33cb763 into NVIDIA:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzient mzient mzient approved these changes

@JanuszL JanuszL JanuszL approved these changes

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
4 participants
@jantonguirao @dali-automaton @mzient @JanuszL