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

Rotate operator using Warp kernel #1403

Merged
merged 6 commits into from
Oct 23, 2019
Merged

Rotate operator using Warp kernel #1403

merged 6 commits into from
Oct 23, 2019

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Oct 18, 2019

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Why we need this PR?

  • It adds ability to calculate canvas sizes
  • It adds per-sample explicit canvas size to rotate
  • It improves performance.

What happened in this PR?

  • add AdjustParams to WarpParamProvider
  • factor out WarpAttr schema
  • add deg/rad conversion to math_util

JIRA TASK: [DALI-1095]

@mzient
Copy link
Contributor Author

mzient commented Oct 18, 2019

!build

@mzient mzient changed the title Rotate operator using Warp kernel [WIP] Rotate operator using Warp kernel Oct 18, 2019
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [951222]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [951222]: BUILD FAILED

- use AffineMapping
- calculate output canvas size

New:
- Rotate operator
- RotateParamProvider

Changes:
- add AdjustParams to WarpParamProvider
- factor out WarpAttr schema
- add deg/rad conversion to math_util

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
…at the augmentations really did.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
…center.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Oct 22, 2019

!build

Add comments to augmentation gallery.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [955335]: BUILD STARTED

@mzient mzient changed the title [WIP] Rotate operator using Warp kernel Rotate operator using Warp kernel Oct 22, 2019
R"code(Counterclockwise rotation angle, in degrees.)code", DALI_FLOAT, true)
.InputLayout(0, "HWC")
.AddParent("DisplacementFilter");
.DocStr(R"code(Apply an affine transformation to the image.)code")
Copy link
Contributor

Choose a reason for hiding this comment

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

Something more specific about Rotate?

dali/test/python/test_operator_rotate.py Show resolved Hide resolved
" input_type: ", itype,
" output_type: ", otype)
cv_pipeline = CVPipeline(batch_size, otype, itype, output_size);
cv_pipeline.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

build() will be invoked in compare_pipelines so you don't need to call it here

for batch_size in [1, 4, 19]:
for output_size in [None, (160,240)]:
for (itype, otype) in io_types:
print("Testing cpu vs cv",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use yield you'll get a print of the parameters automatically

" input type: ", itype,
" output type: ", otype)
cpu_pipeline = RotatePipeline("cpu", batch_size, otype, itype, output_size);
cpu_pipeline.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

#include "dali/core/format.h"

namespace dali {
namespace rotate_impl {
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
namespace rotate_impl {
namespace rotate {
namespace impl {

maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A shy 'no'? I don't think that level of nesting is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a matter of fact, I'll remove this namespace and have the using directives in the function that needs them.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [955335]: BUILD FAILED

Fix review issues.
Reorganize tests.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Oct 22, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [955504]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [955504]: BUILD PASSED

@mzient mzient merged commit 26a927a into NVIDIA:master Oct 23, 2019
rbetz pushed a commit to rbetz/DALI that referenced this pull request Oct 28, 2019
* Breaking (?) changes:
  - `Rotate` now calculates output size to fit the rotated image *by default*
  - Interpolation method now defaults to INTERP_LINEAR
* New Rotate operator based on Warp operator base:
  - use AffineMapping
  - infer output size
* Remove old Rotate
* Remove CropMirrorNormalize from augmentation_gallery - it obscured what the augmentations really did.
* Remove Rotate test from `test_pipeline.py`
* Add standalone rotate python test with OpenCV reference

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

4 participants