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

Linear Transformation kernel for CPU #1300

Merged
merged 127 commits into from
Oct 4, 2019
Merged

Linear Transformation kernel for CPU #1300

merged 127 commits into from
Oct 4, 2019

Conversation

szalpal
Copy link
Member

@szalpal szalpal commented Sep 25, 2019

Why we need this PR?
Because we have LinearTransformationGpu kernel. That's for consistency

Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Comment on lines 488 to 502
template <int rows, int cols, typename T = float>
DALI_HOST_DEV constexpr dali::mat<rows, cols, T> eye() {
mat<rows, cols, T> ret(0);
for (int i = 0; i < rows && i < cols; i++) {
ret(i, i) = 1;
}
return ret;
}


template <int N, typename T = float>
DALI_HOST_DEV constexpr dali::mat<N, N, T> eye() {
return eye<N, N, T>();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These functions are redundant. Construction from scalar puts that scalar on the diagonal.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in my opinion baaaad. Is there any FW or library, that does the diagonal-initialization by the constructor? I'd rather expect the construtor to initialize all the matrix elements with given value (cf. OpenCV). I'd vote for chaning the behavoiur of the constructor, but not in this PR, since this change can create problems...

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what you have in OpenGL / shader programming.
Since matrices are usually used for multiplaction and vectors for addition, this gives consistent results:

mat3 A;
A * 5 == A * mat3(5);
vec4 v;
v + 5 = v + vec4(5);


template <int rows, int cols, typename T>
std::ostream &operator<<(std::ostream &os, const dali::mat<rows, cols, T> &m) {
for (int i = 0; i < rows; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Side-note: we'd certainly want a prettier printer for matrices, but it'll do for the sake of this PR.

Comment on lines 411 to 433

TEST(MatTest, identity_matrix) {
mat3 three_by_three = {{
{1, 0, 0},
{0, 1, 0},
{0, 0, 1}
}};
mat3x1 three_by_one = {{
{1},{0},{0} // NOLINT
}};
mat3x2 three_by_two = {{
{1, 0},
{0, 1},
{0, 0},
}};
auto m1 = eye<3, int>();
auto m2 = eye<3, 1, int>();
auto m3 = eye<3, 2, int>();
EXPECT_EQ(m1, three_by_three);
EXPECT_EQ(m2, three_by_one);
EXPECT_EQ(m3, three_by_two);
}

Copy link
Contributor

@mzient mzient Oct 3, 2019

Choose a reason for hiding this comment

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

Redundant!

Copy link
Contributor

@mzient mzient left a comment

Choose a reason for hiding this comment

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

Remove "eye" functions.
Use roi.hi instead lo+extent.

Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal
Copy link
Member Author

szalpal commented Oct 3, 2019

Remove "eye" functions.

I don't really agree, that eye function is redundant. What you suggest is that instead of having eye, we can use a constructor. Let's look at the ctors for matrix-types in various libraries.
As for mat constructors, typically there are these approaches:

  1. Matlab, OpenCV, Numpy -like
mat my_mat = 4;  // generates matrix with all elements set to 4
  1. GLSL
mat3 my_mat = mat3(5.0);  // generates matrix with elements in diagonal set to 5.0

Note: I have no idea, if you can do mat3 my_mat = 3.0; in GLSL.
Note2: AFAIK, in GLSL there's no such constructor for vector types

Clearly, more common approach is the OpenCV-like, i.e. constructor generates matrix with all elements set to given value. It gives consistency between matrix-type and vector-type (GLSL doesn't contain such ctor for vector). Therefore in my opinion, our mat constructor should follow OpenCV approach.

Getting back to the eye function. I understand that such constructor already exists (in my opinion it should be changed), so I don't push on changing it. But I don't agree, that in such case eye is redundant. Such function exists in all, OpenCV, Numpy and Matlab, so whoever used those more that OpenGL (which I guess is much more common case amongst deep learning developers), he'll be used to have eye, not a diagonal-based constructor.

Lastly, how does it hurt?

Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
include/dali/core/geom/mat.h Outdated Show resolved Hide resolved
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal szalpal requested a review from mzient October 4, 2019 07:50
include/dali/core/geom/mat.h Outdated Show resolved Hide resolved
dali/core/geom_mat_test.cu Outdated Show resolved Hide resolved
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal szalpal requested a review from mzient October 4, 2019 10:51
@szalpal
Copy link
Member Author

szalpal commented Oct 4, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930370]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930370]: BUILD FAILED

Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal
Copy link
Member Author

szalpal commented Oct 4, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930398]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930398]: BUILD FAILED

Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal
Copy link
Member Author

szalpal commented Oct 4, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930529]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [930529]: BUILD PASSED

@szalpal szalpal merged commit ce33fcd into NVIDIA:master Oct 4, 2019
@szalpal szalpal deleted the lintrans_cpu branch November 20, 2019 11:19
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.

6 participants