Skip to content

G-API: Documentation for Params (IE and ONNX).#20112

Merged
alalek merged 7 commits intoopencv:masterfrom
mpashchenkov:mp/ocv-gapi-docs-part1
Jun 9, 2021
Merged

G-API: Documentation for Params (IE and ONNX).#20112
alalek merged 7 commits intoopencv:masterfrom
mpashchenkov:mp/ocv-gapi-docs-part1

Conversation

@mpashchenkov
Copy link
Copy Markdown
Contributor

@mpashchenkov mpashchenkov commented May 18, 2021

Documentation for cfg_functions of Params.

Docs: http://pullrequest.opencv.org/buildbot/export/pr/20112/docs/

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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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

Magic commands:

force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

@alalek
Copy link
Copy Markdown
Member

alalek commented May 18, 2021

BTW, In documentation PRs it makes sense to update PR's description with link on generated affected documentation pages (after "Docs" builder is completed). Link should point somewhere here: http://pullrequest.opencv.org/buildbot/export/pr/20112/docs/

This is extremely useful for reviewers.

@dmatveev dmatveev self-assigned this May 20, 2021
Copy link
Copy Markdown
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.

Good start! Need to make it a little bit more comprehensive

@mpashchenkov mpashchenkov force-pushed the mp/ocv-gapi-docs-part1 branch from f3a401a to d23d02b Compare May 26, 2021 11:58
};

Params<Net>& cfgInputLayers(const typename PortCfg<Net>::In &ll) {
/** @brief Specifies sequence of CNN input layers names for inference.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wouldn't hardcode to CNN networks, it works with any networks right ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed. network instead CNN.

Params& pluginConfig(const IEConfig& cfg) {
desc.config = cfg;
/** @overload
Function with an rvalue parameter.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

an -> a

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

std::string device_id; //!< Device specifier.

// NB: Here order follows the `Net` API
std::vector<std::string> input_names; //!< Names of input CNN layers.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Names of input network layers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would stop using CNN for all entities below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Copy Markdown
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.

Please align the ONNX documentation with the feedback on the IE stuctures (hide paramdesc, etc)

}

/** @overload
Function for reshape by image size.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function doesn't reshape any images

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment on lines +293 to +295
The function is used to set names of layers that will be used for network reshape.
Dimensions will be constructed automatically by current network input and height and
width of image.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please align with the rest of the reshape documentation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done

Comment on lines +306 to +310
Function with a rvalue parameters for reshape by image size.

@param layer_names rvalue set of the selected layers will be reshaped automatically
its input image size.
@return the reference on modified object.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to document the rvalue specifics, simply mention the overload.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done

@dmatveev
Copy link
Copy Markdown
Contributor

dmatveev commented Jun 7, 2021

@mpashchenkov is there any updates atop of the last (6d ago) review?

Copy link
Copy Markdown
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.

@TolyaTalamanov please review my comments carefully.

@TolyaTalamanov TolyaTalamanov force-pushed the mp/ocv-gapi-docs-part1 branch from 0da717b to 74d5454 Compare June 9, 2021 08:00
Copy link
Copy Markdown
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.

Thanks a lot! Please go ahead with the merge (cc: @alalek )

@alalek alalek merged commit 8e386ac into opencv:master Jun 9, 2021
@alalek alalek mentioned this pull request Jun 13, 2021
@alalek alalek mentioned this pull request Oct 15, 2021
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
G-API: Documentation for Params (IE and ONNX).

* Applying comments

* Removed type of model from PramsDesc

* Added message for onnx ParamDesc

* Whitespaces

* Review

* Fix comments to review

* Fix comments

Co-authored-by: Anatoliy Talamanov <anatoliy.talamanov@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants