Skip to content

Fixed removing is_parameter, is_constant, is_output#17888

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
ilyachur:feature/ichuraev/1324_2
Jul 21, 2020
Merged

Fixed removing is_parameter, is_constant, is_output#17888
opencv-pushbot merged 1 commit intoopencv:masterfrom
ilyachur:feature/ichuraev/1324_2

Conversation

@ilyachur
Copy link
Copy Markdown
Contributor

@ilyachur ilyachur commented Jul 20, 2020 •
edited
Loading

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 OpenCV (BSD) 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

@ilyachur
Copy link
Copy Markdown
Contributor Author

ilyachur commented Jul 20, 2020

@dkurt Please take a look!

!!! This PR should be merged at the same moment with openvinotoolkit/openvino#1324

@alalek
Copy link
Copy Markdown
Member

alalek commented Jul 20, 2020

@ilyachur @ilya-lavrenov

the same moment

In general, this doesn't work in that way.

Right process of public API change is below:

  • add new API. But don't remove old one (provide compatibility shims for old API), mark old API deprecated. Merge that patch.
  • next, some timeframe for updating all* dependent code (like OpenCV), which should migrate on new API only.
  • finally, remove old API.

@ilyachur
Copy link
Copy Markdown
Contributor Author

ilyachur commented Jul 20, 2020 •
edited
Loading

In general, this doesn't work in that way.

@alalek Does it means that I should split my PR on OpenVINO side on two parts, add new methods after that merge this PR to OpenCV and only after that remove these methods in the OpenVINO?

I thought that I can use product config to validate these changes.

@alalek
Copy link
Copy Markdown
Member

alalek commented Jul 20, 2020

Yes.

OV product config is for testing/validation only. It can't help with merge.

@ilyachur
Copy link
Copy Markdown
Contributor Author

ilyachur commented Jul 20, 2020

Ok, I will revert removed methods in order to merge this PR.

!!! This pr can be merged only after this PR: openvinotoolkit/openvino#1324

@ilyachur ilyachur force-pushed the feature/ichuraev/1324_2 branch from abb7ddc to aaf65bb Compare July 21, 2020 05:46
@dkurt
Copy link
Copy Markdown
Member

dkurt commented Jul 21, 2020

Tested with IE master branch (openvinotoolkit/openvino@f7d6711) - works

dkurt
dkurt approved these changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dkurt dkurt left a comment

Choose a reason for hiding this comment

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

👍

@mshabunin mshabunin added the backport is needed Label for maintainers. Authors of PR can ignore this label Jul 21, 2020
@opencv-pushbot opencv-pushbot merged commit 11fb04c into opencv:master Jul 21, 2020
@ilyachur ilyachur deleted the feature/ichuraev/1324_2 branch July 21, 2020 11:57
@alalek alalek mentioned this pull request Aug 2, 2020
Merged
@dkurt dkurt mentioned this pull request Aug 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkurt dkurt dkurt approved these changes

Assignees

@dkurt dkurt

Labels

category: dnn port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Milestone

4.5.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ilyachur @alalek @dkurt @opencv-pushbot @mshabunin