-
Notifications
You must be signed in to change notification settings - Fork 7k
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
[documentation] video API documentation and wrapper #2778
Conversation
…n into bkorbar/videoAPI/pythonreg
Codecov Report
@@ Coverage Diff @@
## master #2778 +/- ##
==========================================
+ Coverage 73.18% 73.45% +0.27%
==========================================
Files 96 96
Lines 8417 8541 +124
Branches 1316 1338 +22
==========================================
+ Hits 6160 6274 +114
- Misses 1857 1873 +16
+ Partials 400 394 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I've left a set of comments, let me know what you think
if _HAS_VIDEO_OPT: | ||
|
||
class Video: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a follow-up PR: given that we are wrapping our C++ class in a python class, we don't need to have this as is, and we can instead implement everything outside the guard, but have a guard at instantiation time.
Something like
if _HAS_VIDEO_OPT:
def _has_video_opt():
return True
else:
def _has_video_opt():
return False
class Video:
def __init__(self, path, stream):
# check if video_opt is available
if not _has_video_opt():
raise RuntimeError(...)
...
In here, we create a function _has_video_opt
to make the class work with torchscript (which doesn't support globals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to the list of issues to address
Co-authored-by: Francisco Massa <[email protected]>
Co-authored-by: Francisco Massa <[email protected]>
…o bkorbar/docs/videoapi
This reverts commit bd1a3dd.
This reverts commit afef7df.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* initial API documentation attempt * test the docs * initial commit * updating test to match the registration * adding the warning on unsucessful import * Try to do conditional import * Simple fix? * clearing up docs * docstring commit * Adding types in arguments Co-authored-by: Francisco Massa <[email protected]> * reverting warning commit * addressing Francisco's comments * Apply suggestions from code review Co-authored-by: Francisco Massa <[email protected]> * Revert "reverting warning commit" This reverts commit bd1a3dd. * Revert "adding the warning on unsucessful import" This reverts commit afef7df. * remove warnings import Co-authored-by: Francisco Massa <[email protected]>
* initial API documentation attempt * test the docs * initial commit * updating test to match the registration * adding the warning on unsucessful import * Try to do conditional import * Simple fix? * clearing up docs * docstring commit * Adding types in arguments Co-authored-by: Francisco Massa <[email protected]> * reverting warning commit * addressing Francisco's comments * Apply suggestions from code review Co-authored-by: Francisco Massa <[email protected]> * Revert "reverting warning commit" This reverts commit bd1a3dd. * Revert "adding the warning on unsucessful import" This reverts commit afef7df. * remove warnings import Co-authored-by: Francisco Massa <[email protected]>
To be seen if it has impact on performance due to additional python binding