Revise SoftPlus operation reference implementation 37559 by yding10 · Pull Request #5938 · openvinotoolkit/openvino

yding10 · 2021-06-01T01:55:59Z

Details:

improve the input type error check
add invalid input error test case
add backend test case
add attribute test case

Tickets:

37559

ilyachur · 2021-06-02T05:44:58Z

docs/ops/activation/SoftPlus_4.md

-is chosen in such a way that the difference between the linear function and exact calculation is no more than `1e-6`.
-The `threshold` can be calculated with the following formula where `alpha` is the number of digits after the decimal point,
-`beta` is maximum value of *T* data type:
+**Note**: 


@lazarevevgeny Could you please take a look?

I don't agree with these changes. @a-sidorova did analysis and provided a formulae to calculate the threshold based on the data type. We should use this approach and do not hardcode any constants in the specification.

ok, revert back. update after getting alignemnt by email.

ilyachur · 2021-06-02T05:45:33Z

ngraph/core/reference/include/ngraph/runtime/reference/softplus.hpp

            void softplus(const T* arg, T* out, size_t count)
            {
-                const T threshold = static_cast<T>(std::log(std::numeric_limits<T>::max()));
+                const T threshold = 20.0;


Why do you use hardcoded value (Why 20?)?

@ilyachur current implementation is not aligned with spec. threshold is a range that we can choose from, 20 is chosen for FP32 based on experiment, to align to the spec and cpu-plugin implementation. for FP16, it is also working that no overflow will happen as calculation is based on FP32 and converted to FP16 at final stage. So either we change the spec or change the code here to get it aligned.

revert back ..

docs/ops/activation/SoftPlus_4.md

…otoolkit#5892)" This reverts commit 115aa14.

…penvinotoolkit#5892)"" This reverts commit 95afa50.

This reverts commit 91af825.

This reverts commit a3b232a.

ilyachur · 2021-06-08T06:22:19Z

ngraph/core/src/op/softplus.cpp

 void op::v4::SoftPlus::validate_and_infer_types()
 {
    NGRAPH_OP_SCOPE(v4_SoftPlus_validate_and_infer_types);
+    const element::Type& input_et = get_input_element_type(0);


Can you add more tests (for visit_attributes)?
https://github.com/openvinotoolkit/openvino/tree/master/ngraph/test/visitors/op

there is no attribute for this op. the "threshold" we discussed before is an internal defined parameter, which is not exposed to user. is it ok not to add test for visit_attributes?

Yes we need to add tests for such operations too.

Example you can find here: https://github.com/openvinotoolkit/openvino/blob/master/ngraph/test/visitors/op/result.cpp

thanks for reminder, I have added it, please help check it.

@ilyachur is this PR ok to be merged into master?

This reverts commit 610728f.

ilyachur · 2021-06-21T10:37:13Z

ngraph/test/visitors/op/softplus.cpp

+using ngraph::test::NodeBuilder;
+using ngraph::test::ValueMap;
+
+TEST(attributes, softplus)


Hi could you please rebase your PR to the latest master. And please use parametrized visitor API test which was introduced in this PR: #6181

…envinotoolkit#6181

…lkit#5938) * change threshold to 20 instead of max limit of data type * add invalid input type test case * add invalid input data check * add input type dynamic check * add backend test case * add more clarity on spec and align with real implementation * Revert "[CPU] Fix for CoreThreadingTestsWithIterations tests (openvinotoolkit#5892)" This reverts commit 115aa14. * Revert "Revert "[CPU] Fix for CoreThreadingTestsWithIterations tests (openvinotoolkit#5892)"" This reverts commit 95afa50. * Revert "change threshold to 20 instead of max limit of data type" This reverts commit 91af825. * Revert "add more clarity on spec and align with real implementation" This reverts commit a3b232a. * add visitor attribute test case * Revert "add visitor attribute test case" This reverts commit 610728f. * add attribute test case * revise the attribute visitor test per parametrized visitor API PR: openvinotoolkit#6181

change threshold to 20 instead of max limit of data type

91af825

yding10 requested a review from a team June 1, 2021 01:55

yding10 changed the title ~~Revise SoftPlus operation reference implementation~~ Revise SoftPlus operation reference implementation 37559 Jun 1, 2021

openvino-pushbot added ExternalIntelPR External contributor from Intel category: Core OpenVINO Core (aka ngraph) labels Jun 1, 2021

yding10 marked this pull request as draft June 1, 2021 02:20

yding10 added 5 commits June 1, 2021 10:30

add invalid input type test case

e866ba7

add invalid input data check

44de646

add input type dynamic check

5168ad5

add backend test case

f8c12a1

add more clarity on spec and align with real implementation

a3b232a

yding10 marked this pull request as ready for review June 1, 2021 07:44

yding10 requested a review from a team as a code owner June 1, 2021 07:44

yding10 removed the ExternalIntelPR External contributor from Intel label Jun 2, 2021

ilyachur reviewed Jun 2, 2021

View reviewed changes

ilyachur requested a review from lazarevevgeny June 2, 2021 05:46

ilyachur added this to the 2022.1 milestone Jun 2, 2021

yding10 requested a review from a-sidorova June 2, 2021 07:07

lazarevevgeny suggested changes Jun 3, 2021

View reviewed changes

docs/ops/activation/SoftPlus_4.md Outdated Show resolved Hide resolved

Revert "[CPU] Fix for CoreThreadingTestsWithIterations tests (openvin…

95afa50

…otoolkit#5892)" This reverts commit 115aa14.

yding10 requested review from a team June 4, 2021 02:46

yding10 added 3 commits June 4, 2021 10:50

Revert "Revert "[CPU] Fix for CoreThreadingTestsWithIterations tests (o…

b32a32f

…penvinotoolkit#5892)"" This reverts commit 95afa50.

Revert "change threshold to 20 instead of max limit of data type"

4d2f371

This reverts commit 91af825.

Revert "add more clarity on spec and align with real implementation"

948ec77

This reverts commit a3b232a.

yding10 requested review from ilyachur and lazarevevgeny June 7, 2021 07:11

ilyachur reviewed Jun 8, 2021

View reviewed changes

lazarevevgeny approved these changes Jun 8, 2021

View reviewed changes

a-sidorova approved these changes Jun 8, 2021

View reviewed changes

yding10 added 5 commits June 9, 2021 15:01

add visitor attribute test case

610728f

Revert "add visitor attribute test case"

417b78e

This reverts commit 610728f.

Merge branch 'openvinotoolkit:master' into softplus

108c833

add attribute test case

74903c1

Merge remote-tracking branch 'upstream/master' into softplus

0c4bec7

ilyachur approved these changes Jun 15, 2021

View reviewed changes

ilyachur reviewed Jun 21, 2021

View reviewed changes

yding10 added 2 commits June 22, 2021 02:00

Merge remote-tracking branch 'upstream/master' into softplus

8e48f52

revise the attribute visitor test per parametrized visitor API PR: op…

0cf27cd

…envinotoolkit#6181

ilyachur merged commit 5ce5f9e into openvinotoolkit:master Jun 22, 2021

Conversation

yding10 commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details:

Tickets:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yding10 commented Jun 1, 2021 •

edited

Loading