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

Test calculation output shape for Broadcast op, relax restrictions for partially dynamic input data #1247

Merged
merged 17 commits into from
Aug 10, 2020

Conversation

mbencer
Copy link
Contributor

@mbencer mbencer commented Jul 7, 2020

Tested combinations from CVS-31915

@mbencer mbencer self-assigned this Jul 7, 2020
@ilya-lavrenov ilya-lavrenov added the category: Core OpenVINO Core (aka ngraph) label Jul 8, 2020
@ilya-lavrenov ilya-lavrenov added this to the 2021.1 milestone Jul 8, 2020
Copy link
Contributor

@arogowie-intel arogowie-intel left a comment

Choose a reason for hiding this comment

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

I haven't looked through all tests. However after review of spreadsheet with matrix of cases we came to conclusion we should clarify the resulting shapes.
My understanding is that, we should leverage as much as possible information available from target_shape input in case of numpy and explicit broadcasting rules, which I'd like to emphasize are uni-directional modes.

Comment on lines 155 to 156
const bool is_output_rank_static =
output_shape.rank().is_static() && output_shape[0].is_static();
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable name is misleading. The conditions are more restrictive than the variable name. In fact they boil down to output_shape.is_static() - since you've already enforced 1D target shape and is_static function checks whether partial shape rank is static as well as all of its dimensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I changed it to is_target_shape_known

ngraph/src/ngraph/op/util/broadcast_base.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Show resolved Hide resolved
@mbencer
Copy link
Contributor Author

mbencer commented Jul 23, 2020

I haven't looked through all tests. However after review of spreadsheet with matrix of cases we came to conclusion we should clarify the resulting shapes.
My understanding is that, we should leverage as much as possible information available from target_shape input in case of numpy and explicit broadcasting rules, which I'd like to emphasize are uni-directional modes.

You are right. I've changed rules for numpy. Now if target_shape is constant, output_shape is always set (validation is realized if possible - if input data dimensions are known)

@mbencer mbencer marked this pull request as ready for review July 23, 2020 11:45
@mbencer mbencer requested review from a team and lazarevevgeny and removed request for ilya-lavrenov July 23, 2020 11:45
@mbencer mbencer requested a review from arogowie-intel July 23, 2020 13:08
Copy link
Contributor

@arogowie-intel arogowie-intel left a comment

Choose a reason for hiding this comment

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

Just few typos.

ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
ngraph/src/ngraph/op/util/broadcast_base.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@lazarevevgeny lazarevevgeny left a comment

Choose a reason for hiding this comment

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

I have a brief look at the changes. Please, fix some minor comments.

ngraph/src/ngraph/op/broadcast.cpp Show resolved Hide resolved
ngraph/test/eval.cpp Show resolved Hide resolved
ngraph/test/type_prop/broadcast.cpp Outdated Show resolved Hide resolved
@mbencer mbencer requested a review from lazarevevgeny July 29, 2020 12:03
@mbencer mbencer requested a review from jane-intel August 7, 2020 17:31
@postrational postrational merged commit ae48d9d into openvinotoolkit:master Aug 10, 2020
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Aug 26, 2020
RomanZm pushed a commit to RomanZm/openvino that referenced this pull request Aug 28, 2020
@mbencer mbencer deleted the mbencer/BroadcastShapes branch March 4, 2021 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants