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

Decouple visualizer for OTX #356

Merged

Conversation

igor-davidyuk
Copy link
Collaborator

This PR introduces the Visualizer entity

Signed-off-by: Igor Davidyuk <igor.davidyuk@intel.com>
Signed-off-by: Igor Davidyuk <igor.davidyuk@intel.com>
Signed-off-by: Igor Davidyuk <igor.davidyuk@intel.com>
Signed-off-by: Igor Davidyuk <igor.davidyuk@intel.com>
Signed-off-by: Igor Davidyuk <igor.davidyuk@intel.com>
Copy link
Collaborator

@ljcornel ljcornel left a comment

Choose a reason for hiding this comment

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

PR looks good, just some minor comments

"""
Visualize the predicted output by drawing the annotations on the input image.

:example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering about this example, I think it'd be better to change it to use the Deployment API that we have in the SDK. This looks like it still shows the OTX api for prediction


:param image: Image to be shown.
"""
if not self.no_show:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems weird, I'd say we should at least log that .show has been called while the interactive output was suppressed. Otherwise it might leave a user wondering why they are not seeing any output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it looks odd 😁
This parameter is there for the future when Visualizer will be able to save images without showing them.
The logging should be there, I agree. But as it is not used yet, I will just remove this one for now

@igor-davidyuk igor-davidyuk merged commit d87fd85 into openvinotoolkit:main Mar 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants