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

[onert] Apply ReLU6Grad to ElementwiseActivationLayer #12487

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Jan 17, 2024

This PR applies ReLU6Grad to ElementwiseActivationLayer.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn [email protected]

issue : #12388
draft : #12395

Comment on lines +63 to +70
auto relu_cker = [&alpha]() {
if (alpha == std::numeric_limits<float>::infinity())
return nnfw::cker::train::ReLUGrad;
else if (alpha == 6.0f)
return nnfw::cker::train::ReLU6Grad;
else
throw std::runtime_error{"no supported relu kernel"};
}();
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) select kernel by alpha value

};
}
else
{
throw std::runtime_error("train ElementwiseActivationLayer : This layer does not "
"suppport other ReLU except for ReLU(0-inf)");
"suppport other ReLU except for ReLU(0-inf) and ReLU6(0-6)");
}
Copy link
Contributor Author

@zetwhite zetwhite Jan 17, 2024

Choose a reason for hiding this comment

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

NOTE

After This PR, the training feature supports (not fused) ReLU6.

I also checked loss value isn't different with tensorflow.

  • tested model :

    image

  • TensorFlow

Epoch 1/5
50/50 [==============================] - 0s 617us/step - loss: 0.0794 - mean_squared_error: 0.0794
Epoch 2/5
50/50 [==============================] - 0s 542us/step - loss: 0.0586 - mean_squared_error: 0.0586
Epoch 3/5
50/50 [==============================] - 0s 533us/step - loss: 0.0505 - mean_squared_error: 0.0505
Epoch 4/5
50/50 [==============================] - 0s 542us/step - loss: 0.0461 - mean_squared_error: 0.0461
Epoch 5/5
50/50 [==============================] - 0s 513us/step - loss: 0.0431 - mean_squared_error: 0.0431
  • onert_train
- learning_rate   = 0.001
- batch_size      = 20
- loss_info       = {loss = mean squared error, reduction = sum over batch size}
- optimizer       = adam
========================
Epoch 1/5 - time: 3.351ms/step - loss: [0] 0.0794
Epoch 2/5 - time: 3.375ms/step - loss: [0] 0.0586
Epoch 3/5 - time: 3.409ms/step - loss: [0] 0.0505
Epoch 4/5 - time: 3.655ms/step - loss: [0] 0.0461
Epoch 5/5 - time: 3.915ms/step - loss: [0] 0.0431
===================================

Comment on lines +72 to 77
_backward_kernel = [relu_cker](const IPortableTensor *output,
const IPortableTensor *incoming,
IPortableTensor *outgoing) {
relu_cker(getShape(output), getBuffer<float>(output), getShape(incoming),
getBuffer<float>(incoming), getShape(outgoing), getBuffer<float>(outgoing));
};
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) create _backend_kernel, by capturing selected relu_cker

@zetwhite zetwhite requested a review from ragmani January 17, 2024 05:56
@zetwhite zetwhite added approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it. labels Jan 17, 2024
@zetwhite zetwhite requested a review from a team January 17, 2024 05:56
ragmani
ragmani previously approved these changes Jan 17, 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

@zetwhite zetwhite requested a review from a team January 17, 2024 06:20
};
}
else
{
throw std::runtime_error("train ElementwiseActivationLayer : This layer does not "
"suppport other ReLU except for ReLU(0-inf)");
"suppport other ReLU except for ReLU(0-inf) and ReLU6(0-6)");
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) It would be good not to list the supported ReLU types here. We need to update the message whenever supported list is updated. What about something like "Not supported ReLU type activation".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks better!
updated :)

glistening
glistening previously approved these changes Jan 17, 2024
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

jyoungyun
jyoungyun previously approved these changes Jan 17, 2024
Copy link
Contributor

@jyoungyun jyoungyun 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 Jan 17, 2024
This PR applies ReLU6Grad to ElementwiseActivationLayer.

ONE-DCO-1.0-Signed-off-by: SeungHui Youn <[email protected]>
return nnfw::cker::train::ReLU6Grad;
else
throw std::runtime_error{"no supported relu kernel"};
}();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
}();
}();

lambda is invoked here.
and it returns a function pointer to relu_cker.

Copy link
Contributor

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

auto relu_cker = [&alpha]() {
if (alpha == std::numeric_limits<float>::infinity())
return nnfw::cker::train::ReLUGrad;
else if (alpha == 6.0f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. Is it okay to compare float value with float value? I'm not sure....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain a little more?

At first glance, I failed to catch your worries.
I thought It is natural to compare one float with another.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally not, but some range of integer is exactly represented in float format.

For float, it has 24-bit significand. 6 is exactly representalbe.

To make sure,

https://en.wikipedia.org/wiki/Single-precision_floating-point_format

Integers between 0 and 16777216 can be exactly represented

Copy link
Contributor

@YongseopKim YongseopKim Jan 18, 2024

Choose a reason for hiding this comment

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

Sorry for not enough explaination 😥

I'm asking about approximate equality

Q.

(0.0f == 0.0f) // true or false?

Copy link
Contributor

@YongseopKim YongseopKim Jan 18, 2024

Choose a reason for hiding this comment

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

Almost it returns true but rarely could return false. If you already know this and then code it, it would be okay. But if not, we could consider it. 😀

Copy link
Contributor

@ragmani ragmani Jan 18, 2024

Choose a reason for hiding this comment

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

As far as I know, (6.0f == 6.0f) is always true. They have the same precision since they are float literal.
But when one of them is a double or a long double like (6.0f == 6.0), I'm not sure if they have the same precision.
So I tested it.

#include <iomanip>
#include <iostream>
#include <limits>
#include <typeinfo>
 
#define OUT(x) '\n' << std::setw(16) << #x << x
 
int main()
{
    std::cout
        << "Literal" "\t" "Printed value" << std::left
        << std::setprecision(39)
        << OUT( 3.4028234e38f  ) // float
        << OUT( 3.4028234e38   ) // double
        << OUT( 3.4028234e38l  ) // long double
        << OUT( 6.0f  ) // float
        << OUT( 6.0   ) // double
        << OUT( 6.0l  ) // long double
        << '\n';
}
Literal	Printed value
3.4028234e38f   340282346638528859811704183484516925440
3.4028234e38    340282339999999992395853996843190976512
3.4028234e38l   340282339999999999995912555211526242304
6.0f            6
6.0             6
6.0l            6

It seems that both exponent value and fraction value of 6.0 and 6.0f are the same.

  • Dump
    6.0f : 0xc0c00000
    6.0 : 0xc018000000000000

Copy link
Contributor

@YongseopKim YongseopKim Jan 18, 2024

Choose a reason for hiding this comment

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

Hmm... maybe your concern is 6 may be represented in different way for lhs and rhs. Is it right?

To clarify, not concern but curious.

And my point is comparing mantissa of floating value and mantissa of another floating value.

Copy link
Contributor

@ragmani ragmani Jan 18, 2024

Choose a reason for hiding this comment

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

Hmm... maybe your concern is 6 may be represented in different way for lhs and rhs. Is it right?

To clarify, not concern but curious.
And my point is comparing mantissa of floating value and mantissa of another floating value.

I updated about it "It seems that both exponent value and fraction value of 6.0 and 6.0f are the same" in https://github.com/Samsung/ONE/pull/12487#discussion_r1456812278. In short, if comparing value is 6.0, there is no problem.
Is your curiousity resolved?

Copy link
Contributor

@YongseopKim YongseopKim Jan 18, 2024

Choose a reason for hiding this comment

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

And I already told the term approximate equality.

if we have a consensus, because of value 6, it is okay.

I was curious what sth happens if the floating point's precision is too big.

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is not pointing out that this is a problem.

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

@glistening glistening merged commit 908a3be into Samsung:master Jan 18, 2024
3 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.

5 participants