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.

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

Use resampling in both RandomResizedCrop and Resize #642

Merged
merged 5 commits into from Mar 15, 2019

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Mar 12, 2019

No description provided.


attr_output.Resize(Dims{2});
int *t = attr_output.mutable_data<int>();
t[0] = out_shape[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was a bug. GPU variant uses input size, which seems more reasonable.

@mzient mzient force-pushed the FusedResizeOps branch 2 times, most recently from 67ff1de to dd990dc Compare March 12, 2019 17:53
Copy link
Contributor

@JanuszL JanuszL left a comment

Choose a reason for hiding this comment

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

Please uncomment those two disabled test in the qa/L0_python-self-test/test.sh.

@@ -170,6 +171,18 @@ view_as_tensor(const TensorList<Backend> &data) {
return { data.template data<U>(), tensor_shape<ndim>(data) };
}

template <int ndim>
void to_dims_vec(std::vector<Dims> &dims_vec, const kernels::TensorListShape<ndim> &tls) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not returning the vector ?
And if an output variable, I think that it'd be more intuitive to have the outputs at the end of the arguments

Copy link
Contributor Author

@mzient mzient Mar 13, 2019

Choose a reason for hiding this comment

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

I don't want to have a dynamic allocation in steady code path. As for the argument order, I always follow memcpy convention.

DALI_INTERP_LINEAR)
.AddOptionalArg("mag_filter", "Filter used when scaling up",
Copy link
Contributor

Choose a reason for hiding this comment

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

if several operators are using those resampling related arguments you could register a schema ResamplingAttr (just the schema, no operator) and just write here AddParent("ResamplingAttr").
You can look at what I did with CropAttr or RandomCropAttr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll see how it works out.

@@ -52,8 +61,8 @@ void ResizeBase::GetResamplingFilters(kernels::FilterDesc &min_filter,
else if (spec.HasArgument("interp_type"))
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you rely on default arguments instead of initializing to default and then checking if HasArgument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to know if the argument has been specified or not. If it has, it overrides the value defined by interp_type. It's different than relying on default value, in which case I wouldn't know if the value was specified, but was assigned the default value explicitly, or was not defined at all (in which case interp_type should be used, if defined). The logic is a bit convoluted, but it preserves the old behavior if interp_type is set.

@mzient mzient force-pushed the FusedResizeOps branch 2 times, most recently from 147804b to fd093f7 Compare March 13, 2019 10:32
Refactor RandomResizedCrop and Resize to have common base worker class.
ResizeBase uses resampling kernels for CPU and GPU.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Add hint for GPU scratch memory.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@JanuszL JanuszL added this to the Release_0.8.0 milestone Mar 14, 2019
@JanuszL JanuszL merged commit 7cefd60 into NVIDIA:master Mar 15, 2019
JanuszL pushed a commit that referenced this pull request Mar 15, 2019
* Add min_filter and mag_filter parameters to RandomResizedCrop schema.
* Use resampling in RandomResizedCrop.
* Refactor RandomResizedCrop and Resize to have common base worker class.
* ResizeBase uses resampling kernels for CPU and GPU.
* Add ResamplingFilterAttr, remove duplicate attributes from operators.
* Set output layout in Resize.
* Changed downscaling to linear for the sake of presized datasets.
* Add hint for GPU scratch memory.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
haoxintong pushed a commit to haoxintong/DALI that referenced this pull request Jul 16, 2019
* Add min_filter and mag_filter parameters to RandomResizedCrop schema.
* Use resampling in RandomResizedCrop.
* Refactor RandomResizedCrop and Resize to have common base worker class.
* ResizeBase uses resampling kernels for CPU and GPU.
* Add ResamplingFilterAttr, remove duplicate attributes from operators.
* Set output layout in Resize.
* Changed downscaling to linear for the sake of presized datasets.
* Add hint for GPU scratch memory.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
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.

None yet

3 participants