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

LP Transformation tests use API 2.0 #21677

Merged
merged 38 commits into from
Jan 18, 2024

Conversation

evkotov
Copy link
Contributor

@evkotov evkotov commented Dec 14, 2023

Details:

  • use SubgraphBaseTest as the base class
  • rename Run -> run
  • add init_input_shapes

Tickets:

  • 111374

@evkotov evkotov self-assigned this Dec 14, 2023
@evkotov evkotov requested review from a team as code owners December 14, 2023 19:11
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Dec 14, 2023
@evkotov evkotov requested review from a team as code owners December 18, 2023 17:31
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Dec 18, 2023
@evkotov evkotov changed the title [NO REVIEW] [NO MERGE] LP Transformation tests use API 2.0 LP Transformation tests use API 2.0 Dec 20, 2023
@vurusovs vurusovs self-assigned this Dec 25, 2023
@eshoguli
Copy link
Contributor

@iefode, note, please, in some test cases we need to set rel_threshold more then 100%. Is it expected? We need to do this because current tensor comparision has specific implementation for zero values.

inputShape2[3] = 1;
}
init_input_shapes({ inputShape, inputShape2 });

function = ngraph::builder::subgraph::AddFunction::getOriginal(
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use ngraph. Check similar usage of ngraph functions in modified files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itikhono,
There are helper functions in src/tests/ov_helpers/ov_lpt_models. Should it be fixed ngraph -> ov namespace in this PR or it could be done in next separate PR? There are already 200+ files in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline. This will be done in a next separate PR since there are already too many files in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created ticket

@iefode iefode self-assigned this Jan 8, 2024
const float min,
const float max,
const int seed) {
std::mt19937 gen(seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just minor open question: to use such generator for instead of gtest::Random?
@evkotov Have you checked which generator is using in gtest::Random?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main problem to generate within [min, max] range with float min and max. It is simple to get the way I used. gtest::Random operates uint32 range

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor for the future: unite two functions into one. Because float numbers could be generate by resolution using

@sbalandi
Copy link
Contributor

@iefode, note, please, in some test cases we need to set rel_threshold more then 100%. Is it expected? We need to do this because current tensor comparision has specific implementation for zero values.

Hi @eshoguli , if you mean, that tests fail in case threshold and max error are equal, this behavior is fixed in PR: #21783 (function less_or_equal in src/tests/test_utils/common_test_utils/src/ov_tensor_utils.cpp ). Also, please, not, that after this PR the meaning of rel_threshold will change and we are checking, that accumulated error is not greater than the sum of the maximum allowed errors for all shapes

Copy link
Contributor

@iefode iefode left a comment

Choose a reason for hiding this comment

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

In general LGTM. I have left several comments for the future.


static std::pair<float, float> getQuantizationInterval(const ngraph::element::Type precision);
static std::string to_string(const ov::pass::low_precision::LayerTransformation::Params& params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: so strange naming... What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks strange in diff. If you open file you can see that getQuantizationInterval renamed to get_quantization_interval

}

void LayerTransformation::init_input_shapes(const ov::PartialShape& shape) {
std::pair<ov::PartialShape, std::vector<ov::Shape>> input_shapes(shape, { shape.to_shape() });
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not get any response to the comment. Why should it be resolved?

const float min,
const float max,
const int seed) {
std::mt19937 gen(seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor for the future: unite two functions into one. Because float numbers could be generate by resolution using

Copy link
Contributor

@jane-intel jane-intel left a comment

Choose a reason for hiding this comment

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

No significant comments from my side. You did a great job cleaning out ngraph namespace usage. I'm looking forward for the future clearing of API 1.0 headers and methods.
Please elaborate why some tests are skipped and some tests are given huge tolerance in the description of this PR. It seems like we are losing the coverage just because we are switching namespaces.
Overall, looks ok to me.

Comment on lines -45 to +44
// threshold = 0.1f;
abs_threshold = 1.0e-3;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it desirable behavior? did we lose coverage? did we soften the testing?

@@ -31,12 +30,15 @@ std::string ReduceMaxTransformation::getTestCaseName(const testing::TestParamInf
}

void ReduceMaxTransformation::SetUp() {
ngraph::element::Type netPrecision;
ngraph::PartialShape inputShape;
abs_threshold = 1.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like a big threshold. could you elaborate what was the original threshold for this test before refactoring.

itikhono and others added 3 commits January 18, 2024 12:17
Co-authored-by: Evgenya Nugmanova <[email protected]>
…low_precision_transformations/fuse_multiply_to_fq_transformation.cpp

Co-authored-by: Evgenya Nugmanova <[email protected]>
…low_precision_transformations/fuse_subtract_to_fq_transformation.cpp

Co-authored-by: Evgenya Nugmanova <[email protected]>
@itikhono
Copy link
Contributor

Looks good!
Let's address the remaining comments in the next PR and merge this one today.

@itikhono itikhono enabled auto-merge (squash) January 18, 2024 08:20
@itikhono itikhono merged commit 0dc2dc5 into openvinotoolkit:master Jan 18, 2024
99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin category: GPU OpenVINO GPU plugin category: IE Tests OpenVINO Test: plugins and common conformance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants