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

G-API: API for configuring model output precision for IE backend #22583

Conversation

TolyaTalamanov
Copy link
Contributor

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

Overview

This cfgOutputPrecision handle is partially introduced/discussed there: #22522

@TolyaTalamanov
Copy link
Contributor Author

@smirnov-alexey Could you have a look, please?


The function is used to set an output precision for model.

@param precision Precision in OpenCV format.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to explain what is the opencv format.
@alalek Could you explain how to point to opencv types: https://docs.opencv.org/4.x/d1/d1b/group__core__hal__interface.html#ga32b18d904ee2b1731a9416a8eef67d06 in doxygen.

Copy link
Contributor

Choose a reason for hiding this comment

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

(CV_8U, CV_32F, ...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const ParamDesc::precision_variant_t &output_precision) {
switch (output_precision.index()) {
case ParamDesc::precision_variant_t::index_of<ParamDesc::precision_t>(): {
auto precision = toIE(cv::util::get<ParamDesc::precision_t>(output_precision));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

modules/gapi/src/backends/ie/giebackend.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@smirnov-alexey smirnov-alexey left a comment

Choose a reason for hiding this comment

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

Overall looks great, thanks!

@TolyaTalamanov
Copy link
Contributor Author

@dmatveev Could you have a look, please?

@TolyaTalamanov TolyaTalamanov force-pushed the at/add-cfg-output-precision-for-ie-backend branch from 0ea293f to 0166437 Compare October 2, 2022 20:54
Copy link
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

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

Fine with changes but please keep comments in mind

// NB: cv::util::monostate is default value that means precision wasn't specified.
using precision_variant_t = cv::util::variant<cv::util::monostate,
precision_t,
precision_map_t>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please document the cases in this variant?

My understanding (I haven't read the rest of the patch yet) is that precision_t case is for single output layer, right? And map is for multi-output case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, user can do:

params.cfgOutputPrecision(`CV_16F`) // will be applied for all layers.

or specify certain layers:

params.cfgOutputPrecision({{"layer0", CV_16F}}); // only layer0 has changed precision 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

// will be applied for all layers

Well I failed to guess that. :)

@@ -132,6 +141,7 @@ template<typename Net> class Params {
, {}
, {}
, {}
, {}
Copy link
Contributor

Choose a reason for hiding this comment

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

jeez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must be reorganized, I'm afraid of touching it, since it'll cause lot's of changes.


The function is used to set an output precision for model.

@param precision Precision in OpenCV format.
Copy link
Contributor

Choose a reason for hiding this comment

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

(CV_8U, CV_32F, ...) ?

@param precision Precision in OpenCV format.
@return reference to this parameter structure.
*/
Params<Net>& cfgOutputPrecision(detail::ParamDesc::precision_t precision) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ah this mixture of CamelCase and snake_case_t :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -385,7 +419,7 @@ class Params<cv::gapi::Generic> {
const std::string &device)
: desc{ model, weights, device, {}, {}, {}, 0u, 0u,
detail::ParamDesc::Kind::Load, true, {}, {}, {}, 1u,
{}, {}, {}, {}},
{}, {}, {}, {}, {}},
Copy link
Contributor

Choose a reason for hiding this comment

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

jeez.. (2)

@@ -258,6 +258,7 @@ struct InferParams {
std::vector<std::string> input_layers;
std::vector<std::string> output_layers;
std::map<std::string, std::string> config;
cv::util::optional<int> out_precision;
Copy link
Contributor

Choose a reason for hiding this comment

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

So here it is int, not a map of ints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Config has parameter output_precision which is applied for all output layers. There is no need to have per layer precision for tool (so far...)

Comment on lines +366 to +368
if (infer_params.out_precision) {
pp->cfgOutputPrecision(infer_params.out_precision.value());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No multi-output cases supported? What if the user writes such a config? Would it be rejected gracefully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multi-output case is supported. User can only define precision for all output layers. Answered above

const auto ie_type = toCV(desc.getPrecision());
if (ie_type != mat.type()) {
std::stringstream ss;
ss << "Failed while copying blob from IE to OCV: "
Copy link
Contributor

Choose a reason for hiding this comment

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

Failed to copy

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I doubt if multi-line exception text is useful. At least a single-liner is easier to grep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

static void configureOutputPrecision(const IE::OutputsDataMap &outputs_info,
const ParamDesc::precision_variant_t &output_precision) {
switch (output_precision.index()) {
case ParamDesc::precision_variant_t::index_of<ParamDesc::precision_t>(): {
Copy link
Contributor

Choose a reason for hiding this comment

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

y not visit? we have one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@TolyaTalamanov TolyaTalamanov force-pushed the at/add-cfg-output-precision-for-ie-backend branch from 0166437 to a6fbd82 Compare October 3, 2022 08:05
@@ -2956,6 +2956,111 @@ TEST(TestAgeGender, ThrowBlobAndInputPrecisionMismatchStreaming)
}
}

TEST(TestAgeGenderIE, ChangeOutputPrecision)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must be refactoring asap. Adding new test brings lot's of copy-paste stuff.

@TolyaTalamanov
Copy link
Contributor Author

@dmatveev @asmorkalov Do you mind merging it?

@asmorkalov asmorkalov merged commit 7208f63 into opencv:4.x Oct 3, 2022
@alalek alalek mentioned this pull request Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants