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.

Sign up for GitHub

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

Jump to bottom

[Op Conformance] Update compare accuracy function #21783

Merged
merged 1 commit into from
Jan 23, 2024
Merged

[Op Conformance] Update compare accuracy function #21783

merged 1 commit into from
Jan 23, 2024

Conversation

sbalandi
Copy link
Contributor

@sbalandi sbalandi commented Dec 20, 2023 •
edited
Loading

Details:

  • item1
  • ...

Tickets:

@sbalandi sbalandi requested review from a team as code owners December 20, 2023 09:27
@github-actions github-actions bot added category: IE Tests OpenVINO Test: plugins and common category: CPU OpenVINO CPU plugin labels Dec 20, 2023
@sbalandi
Copy link
Contributor Author

Part of PR: #21347 with changes in code of tests.
PR was reverted because of problem with conversion precision, I exclude these changes (furthermore these changes do not have a big impact) and rerun failed post merge check: https://openvino-ci.toolbox.iotg.sclab.intel.com/job/private-ci/job/ie/job/build-linux-macos_arm64/6276/

@iefode
Copy link
Contributor

iefode commented Dec 21, 2023

@sbalandi What is the status of the PR?

@sbalandi
Copy link
Contributor Author

@sbalandi What is the status of the PR?

the failed check is passed, so I think we can merge it

@iefode
Copy link
Contributor

iefode commented Dec 21, 2023

@sbalandi what about failed macos jobs?

@sbalandi
Copy link
Contributor Author

@sbalandi what about failed macos jobs?

I rerun it here based on that pr: https://openvino-ci.toolbox.iotg.sclab.intel.com/job/private-ci/job/ie/job/build-linux-macos_arm64/6276/ and it pass

iefode
iefode reviewed Jan 9, 2024
View reviewed changes
src/tests/test_utils/common_test_utils/src/ov_tensor_utils.cpp
@@ -281,13 +294,33 @@ void compare(const ov::Tensor& expected,
if (abs_threshold == std::numeric_limits<double>::max() && rel_threshold == std::numeric_limits<double>::max()) {
if (sizeof(ExpectedT) == 1 || sizeof(ActualT) == 1) {
abs_threshold = 1.;
rel_threshold = 1.;
Copy link
Contributor

Choose a reason for hiding this comment

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

@sbalandi Please check the changes on all platforms and skip tests per platform (in case some test is failed only on one platform)

@iefode
Copy link
Contributor

iefode commented Jan 9, 2024

In general, PR LGTM. Anyway, we should check all platforms

@iefode iefode changed the title [apiConformance] Update compare accuracy function [Op Conformance] Update compare accuracy function Jan 9, 2024
@iefode
Copy link
Contributor

iefode commented Jan 9, 2024

@eshoguli Please add a comment related to rel_threshold and 0 handling

@github-actions github-actions bot removed the category: CPU OpenVINO CPU plugin label Jan 9, 2024
@sbalandi sbalandi mentioned this pull request Jan 10, 2024
Merged
eshoguli
eshoguli reviewed Jan 11, 2024
View reviewed changes
Copy link
Contributor

@eshoguli eshoguli left a comment

Choose a reason for hiding this comment

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

please fix

src/tests/test_utils/common_test_utils/src/ov_tensor_utils.cpp Outdated
@@ -363,14 +409,14 @@ void compare(const ov::Tensor& expected,
throw std::runtime_error(out_stream.str());
}
double abs = std::fabs(expected_value - actual_value);
double rel = expected_value ? (abs / std::fabs(expected_value)) : abs;
double rel = expected_value && !std::isinf(expected_value) ? (abs / std::fabs(expected_value)) : abs;
Copy link
Contributor

@eshoguli eshoguli Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In my understanding: we can not use relative error if value is zero.

Imagine next situations:
case #1: expected = 0.0, actual = 0.1, rel = 0.1 <= only 10%, it's not correct
case #2: expected = 0.01 (slightly enlarged than in case #1), actual (the same) = 0.1, rel = abs(0.01 - 0.1)/0.01=9 <= 900%

In the case #2 difference is smaller, but rel is much larger then in case #1. It's not correct. Don't use relative error if value is zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, rel error is calculated now as:

double rel =
            expected_value && actual_value && !std::isinf(expected_value) ? (abs / std::fabs(expected_value)) : 0;

@sbalandi sbalandi force-pushed the accuracyNewTry2 branch 7 times, most recently from 7958781 to 9fae912 Compare January 18, 2024 16:34
@sbalandi sbalandi requested review from a team as code owners January 18, 2024 16:34
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Jan 18, 2024
@github-actions github-actions bot removed the category: GPU OpenVINO GPU plugin label Jan 18, 2024
@iefode
Copy link
Contributor

iefode commented Jan 19, 2024

@sbalandi Is PR ready to merge?

@sbalandi
Copy link
Contributor Author

@sbalandi Is PR ready to merge?

not yet, post merge is failed and i try to understand why

@iefode iefode merged commit 3872ccf into openvinotoolkit:master Jan 23, 2024
100 checks passed
@sbalandi sbalandi mentioned this pull request Feb 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 29, 2024
@sbalandi
Revert PR 22972 (#23110)
45ce9e7 
### Details:
 - *Revert #22972
- *Partly revert #21783

### Tickets:
 - *ticket-id*
ceciliapeng2011
ceciliapeng2011 reviewed Mar 4, 2024
View reviewed changes
src/tests/test_utils/common_test_utils/src/ov_tensor_utils.cpp
abs_error.update(abs, i);
rel_error.update(rel, i);
}
abs_error.mean /= shape_size_cnt;
rel_error.mean /= shape_size_cnt;

if (!(less_or_equal(abs_error.max, abs_threshold) && less_or_equal(rel_error.max, rel_threshold))) {
if (!(less_or_equal(abs_error.max, abs_threshold) || less_or_equal(rel_error.mean, rel_threshold))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@iefode This PR leads to some failures of plugin unit test pass. Would you please check again?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ceciliapeng2011 In your case, please update your branch to latest master. Commit was reverted

usstq
usstq reviewed Mar 22, 2024
View reviewed changes
src/tests/test_utils/common_test_utils/src/ov_tensor_utils.cpp
continue;
} else if ((std::isinf(expected_value) || expected_value <= min_type_expected) &&
(std::isinf(actual_value) || actual_value <= min_type_actual)) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::numeric_limits<float>::min() is smallest positive number representable by float, here all negative values can pass the check, which is not correct behavior, maybe you mean std::numeric_limits<float>::lowest() ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eshoguli eshoguli eshoguli left review comments

@ceciliapeng2011 ceciliapeng2011 ceciliapeng2011 left review comments

@iefode iefode iefode approved these changes

@usstq usstq usstq left review comments

Assignees

@iefode iefode

Labels
category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sbalandi @iefode @usstq @ceciliapeng2011 @eshoguli