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

Add CombineTransforms operator #2317

Merged
merged 43 commits into from
Oct 7, 2020

Conversation

jantonguirao
Copy link
Contributor

Why we need this PR?

Pick one, remove the rest

  • It adds new feature needed to combine several affine transforms into one

What happened in this PR?

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

  • What solution was applied:
    Added a new operator CombineTransforms
  • Affected modules and functionalities:
    New op
  • Key points relevant for the review:
    New op implementation
  • Validation and testing:
    Python tests
  • Documentation (including examples):
    Docstr

JIRA TASK: [DALI-1626]

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao changed the title Add CombineTrnasforms operator Add CombineTransforms operator Oct 1, 2020
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
template <typename T, int mat_dim>
using affine_mat_t = mat<mat_dim, mat_dim, T>;

DALI_SCHEMA(CombineTransforms)
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
DALI_SCHEMA(CombineTransforms)
DALI_SCHEMA(TransformCombine)

I know that "combine transforms" is how you'd say it in English, we'll likely end up with "transforms.Combine" anyway


DALI_SCHEMA(CombineTransforms)
.DocStr(R"code(Combines two or more affine transforms.)code")
.NumInput(2, 10)
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
.NumInput(2, 10)
.NumInput(2, 99)

const auto &shape = ws.template InputRef<CPUBackend>(i).shape();
DALI_ENFORCE(shape == in0_shape,
make_string("All input transforms are expected to have the same shape. Got: ",
in0_shape, " and ", shape, "."));
Copy link
Contributor

@JanuszL JanuszL Oct 5, 2020

Choose a reason for hiding this comment

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

Suggested change
in0_shape, " and ", shape, "."));
in0_shape, " and ", shape, " for ", i, " input.".));

Comment on lines 81 to 84
std::iota(input_order_.begin(), input_order_.end(), 0);
if (!reverse_order_) {
std::reverse(input_order_.begin(), input_order_.end()); // descending order
}
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
std::iota(input_order_.begin(), input_order_.end(), 0);
if (!reverse_order_) {
std::reverse(input_order_.begin(), input_order_.end()); // descending order
}
if (!reverse_order_) {
std::iota(input_order_.rbegin(), input_order_.rend(), 0);
} else {
std::iota(input_order_.begin(), input_order_.end(), 0);
}

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
mat = affine_mat_t<T, mat_dim>::identity();
}

for (auto input_idx : input_order_) {
Copy link
Contributor

@mzient mzient Oct 6, 2020

Choose a reason for hiding this comment

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

I think it would be better to swap nesting order. Also, I think that this input_order_ is too much.

next_mat(i, j) = in_view[sample_idx].data[k];
}
}
mat = next_mat * mat; // mat mul
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
mat = next_mat * mat; // mat mul
mat = reverse_ ? mat * next_mat : next_mat * mat; // mat mul

This obviates the need for input_order_ and the condition should not pose too much overhead. With reversed loop nesting order, mat could be a local variable that's stored immediately in the output (no need for matrices).

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
for (int input_idx = 0; input_idx < ws.NumInput(); input_idx++) {
auto &in = ws.template InputRef<CPUBackend>(input_idx);
auto in_view = view<T>(in);
auto next_mat = affine_mat_t<T, mat_dim>::identity();
Copy link
Contributor

Choose a reason for hiding this comment

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

The last row is never changed, so this can be hoisted outside the loop over inputs.

auto mat = affine_mat_t<T, mat_dim>::identity();
for (int input_idx = 0; input_idx < ws.NumInput(); input_idx++) {
auto &in = ws.template InputRef<CPUBackend>(input_idx);
auto in_view = view<T>(in);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should get these views ones to some vector/smallvector? Now it will involve far too many of vector allocations and copies. Sorry, I haven't noticed that problem when I first suggested reversing the nesting order.

Signed-off-by: Joaquin Anton <janton@nvidia.com>
in_views.reserve(ws.NumInput());
for (int input_idx = 0; input_idx < ws.NumInput(); input_idx++) {
auto &in = ws.template InputRef<CPUBackend>(input_idx);
in_views.emplace_back(view<T, 2>(in));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use emplace only when converting.

Suggested change
in_views.emplace_back(view<T, 2>(in));
in_views.push_back(view<T, 2>(in));

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the combine_transforms branch 5 times, most recently from 6f44266 to 483b5d6 Compare October 6, 2020 16:25
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1682089]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1682089]: BUILD PASSED

@jantonguirao jantonguirao merged commit edbf71b into NVIDIA:master Oct 7, 2020
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