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

[train] Modify maxPool2D in training #14166

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Conversation

icodo98
Copy link
Contributor

@icodo98 icodo98 commented Oct 7, 2024

This commit change casting to static cast.

ONE-DCO-1.0-Signed-off-by: JuYoung Lee [email protected]

This commit change casting to static cast.

ONE-DCO-1.0-Signed-off-by: JuYoung Lee [email protected]
@icodo98 icodo98 added the PR/ready for review It is ready to review. Please review it. label Oct 7, 2024
@icodo98 icodo98 requested review from ragmani and zetwhite October 7, 2024 05:18
@icodo98
Copy link
Contributor Author

icodo98 commented Oct 7, 2024

from : #14149 (comment)

@icodo98 icodo98 changed the title [train] Revise maxPool2D in training [train] Modify maxPool2D in training Oct 7, 2024
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 61 to 64
assert(paddingTop < (1 << 8));
assert(paddingLeft < (1 << 8));
_op_params.padding_values.height = static_cast<uint8_t>(paddingTop);
_op_params.padding_values.width = static_cast<uint8_t>(paddingLeft);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah? It looks like typecasting to int16_t is necessary, not uint8_t.

struct PoolParams
{
PaddingValues padding_values{0, 0};

struct PaddingValues
{
int16_t width;
int16_t height;
};

Copy link
Contributor

@ragmani ragmani Oct 7, 2024

Choose a reason for hiding this comment

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

Tthere is no problem since _op_param.padding_values are filled with zero-extended value from positive numbers of equal range.

  • These padding parameters are always positive number.
  • The positive number range of int16_t is equal to the range of uint8_t. The positive number range of both are not equal. Sorry for the wrong information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha I got it! Thank you for explanation 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

The positive number range of int16_t is equal to the range of uint8_t.

@ragmani Sorry, But I have another question.
Does it mean that int16_t and uint8_t have a same max value?

Copy link
Contributor

@ragmani ragmani Oct 8, 2024

Choose a reason for hiding this comment

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

Sorry, it was the wrong information. They are not the same max value.

  • uint8_t : max value is 1 << 7 - 1
  • int16_t : max value is 1 << 15 - 1

Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

Could you please check which type is appropriate?
#14166 (comment)

zetwhite
zetwhite previously approved these changes Oct 7, 2024
ragmani
ragmani previously approved these changes Oct 7, 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

@icodo98 icodo98 dismissed stale reviews from ragmani and zetwhite via d79a776 October 9, 2024 12:35
This commit updates padding type to int16

ONE-DCO-1.0-Signed-off-by: JuYoung Lee [email protected]
@hseok-oh
Copy link
Contributor

@nnfw-bot test all

@hseok-oh hseok-oh merged commit 1dd7974 into Samsung:master Oct 10, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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