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

TransposeSinking refactoring: part 2 (class names, folders, file names) #16291

Merged
merged 8 commits into from
Mar 15, 2023

Conversation

itikhono
Copy link
Contributor

@itikhono itikhono commented Mar 14, 2023

Details:

All TransposeSinking transformations and tests were moved to transformations/transpose_sinking folder
Introduced ov::pass::transpose_sinking namespace
Transformation classes were renamed: TransposeSinking* -> TS*
Transformation files were renamed: transpose_sinking_* -> ts_*

before: ______________________________________________after: _______________________________
image image

@github-actions github-actions bot added category: TF FE OpenVINO TensorFlow FrontEnd category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: transformations OpenVINO Runtime library - Transformations labels Mar 14, 2023
@itikhono itikhono requested review from evkotov and rkazants March 14, 2023 14:55
@itikhono itikhono added this to the 2023.0 milestone Mar 14, 2023
@itikhono itikhono marked this pull request as ready for review March 14, 2023 14:56
@itikhono itikhono requested review from a team as code owners March 14, 2023 14:56
@itikhono itikhono changed the title TransposeSinking refactoring: part 2 TransposeSinking refactoring: part 2 (class names, folders, file names) Mar 14, 2023
Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

Left one comment about naming and using additional namespace only for particular TS passes.

Other looks good to me

@@ -268,7 +268,7 @@ void FrontEnd::normalize(const std::shared_ptr<ov::Model>& function) const {
manager.register_pass<ov::frontend::tensorflow_lite::pass::TFLQuantizeResolver>();
manager.register_pass<ov::frontend::tensorflow_lite::pass::Rfft2dSimplifier>();
manager.register_pass<ov::pass::TransposeSinking>();
manager.register_pass<ov::pass::TransposeSinkingGeneral>();
manager.register_pass<ov::pass::transpose_sinking::TSGeneral>();
Copy link
Contributor

Choose a reason for hiding this comment

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

To me ov::pass::TransposeSinking looks better. You can include all particular TS passes into transpose_sinking namespace and remain the main entry on pass namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, we already have ov::pass::TransposeSinking (old common transformation), see line 270
We will be able to replace it in the next Release

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what about TransposeSinkingExtended or TransposeSinkingAdvanced names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we can leave both options:
ov::pass::TransposeSinkingGeneral (the alias)
ov::pass::transpose_sinking::TSGeneral

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 will be fixed in the next PR

@itikhono itikhono requested a review from rkazants March 15, 2023 06:59
@itikhono itikhono merged commit e1ee8f0 into openvinotoolkit:master Mar 15, 2023
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Mar 21, 2023
…s) (openvinotoolkit#16291)

* Add descriptions to the transformations, add additional checks

* fix a warning

* TransposeSinking Rafactoring part2: move the transformations to a separate folder, align namespaces

* TransposeSinking refactoring: class names, namespaces

* codestyle

* resolve merge conflicts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: TF FE OpenVINO TensorFlow FrontEnd category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants