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

[cker] Fix min-max calculation in MaxPool2D #14133

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Sep 30, 2024

This PR fixes min-max calculation in MaxPool2D kernel.

ONE-DCO-1.0-Signed-off-by: seunghui youn [email protected]

This PR fixes min-max calculation in MaxPool2D kernel.

ONE-DCO-1.0-Signed-off-by: seunghui youn <[email protected]>
@zetwhite zetwhite requested review from ragmani and icodo98 September 30, 2024 11:16
@zetwhite zetwhite added the approval: 2 Require at least 2 approvals label Sep 30, 2024
@zetwhite zetwhite requested a review from a team September 30, 2024 11:16
out_mat.cwiseMin(params.float_activation_min).cwiseMax(params.float_activation_max);
out_mat = out_mat.cwiseMin(params.float_activation_max).cwiseMax(params.float_activation_min);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note) This line is a main change in this PR

Comment on lines 236 to 278
// with min, max params
{
nnfw::cker::PoolParams op_param;
{
op_param.stride_height = 1;
op_param.stride_width = 1;
op_param.filter_height = 2;
op_param.filter_width = 2;
op_param.padding_values.height = 0;
op_param.padding_values.width = 0;
op_param.float_activation_min = 0.0;
op_param.float_activation_max = 6.0;
}
nnfw::cker::Shape in = {1, 3, 3, 1};
nnfw::cker::Shape out = {1, 2, 2, 1};

MaxPoolOpVerifier<float> verifier(op_param, in, out);

/**
* input(index) : output(arg-index):
*
* 1(0) 2(1) 3(2)
* 4(3) 5(4) 6(5) - (forward) -> 5(4) 6(5)
* 7(6) 8(7) 9(8) 6(7) 6(8)
*/

std::vector<float> input = {1, 2, 3, 4, 5, 6, 7, 8, 9};
std::vector<float> expected_output = {5, 6, 6, 6};
verifier.verifyForward(input, expected_output);

/**
* output_deriv: input_deriv:
* (randomly filled)
*
* 0.1 0.2 0 0 0
* 0.3 0.4 - (backward) -> 0 0.1 0.2
* 0 0.3 0.4
*/

std::vector<float> output_deriv = {0.1, 0.2, 0.3, 0.4};
std::vector<float> expected_input_deriv = {0, 0, 0, 0, 0.1, 0.2, 0, 0.3, 0.4};
verifier.verifyBackward(output_deriv, expected_input_deriv);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note) This is a newly added test - to check that min/max works as expected

@zetwhite zetwhite added the PR/ready for review It is ready to review. Please review it. label Sep 30, 2024
Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

ragmani
ragmani previously approved these changes Sep 30, 2024
Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Jang Jiseob <[email protected]>
Copy link
Contributor

@icodo98 icodo98 left a comment

Choose a reason for hiding this comment

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

LGTM

@zetwhite zetwhite requested a review from a team October 2, 2024 04:19
Copy link
Contributor

@glistening glistening left a comment

Choose a reason for hiding this comment

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

LGTM

Beyond this PR: I've found several typos during reading MaxPool.test.cc:

  • calcuated_grad → calculated_grad
  • cacluated_output → calculated_output

@glistening glistening merged commit aec1335 into Samsung:master Oct 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants