Predict video on local #243
Conversation
2. change predict_video to use ffmpeg instead of moviepy. (moviepy produces 1 extra frame to original video) 3. add unit test code and data for predict_video.py 4. change function name of predict_video to `predict_video_on_local` for clarification
2. add license info regarding ffmpeg-python
|
I have some concerns.
|
There was a problem hiding this comment.
Thanks @jihyeonyi for the effort! I've now gone through the first round of my review.
tests/data/dice/deployment/Detection task/python/model_wrappers/__init__.py
Outdated
Show resolved
Hide resolved
tests/data/dice/deployment/Detection task/python/model_wrappers/__init__.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Samet Akcay <samet.akcay@intel.com>
|
Many thanks for the PR @jihyeonyi. In general it looks good but I'm worried about the installation hassle that the use of I will do a full review of the PR today or tomorrow at the latest, I've been way too busy so apologies for the delay |
|
@ljcornel, I'll take a look at |
examples/predict_video_on_local.py
Outdated
| logging.basicConfig(level=log_level) | ||
|
|
||
| # -------------------------------------------------- | ||
| # Configuration section |
There was a problem hiding this comment.
This is not really in line with the configuration in the other examples. Currently, there is no configuration that the user has to do, because it's all specified in the command line args.
I suggest to make it so that it is possible to run the script against a live Geti instance as well, for which configuration (hostname, credentials, project name) would be required.
From my perspective, there are two use cases for the script:
- The user passes a deployment path, and predictions are generated from there (no server configuration needed)
- The user passes a Geti host, credentials, and a project name. A Deployment is created for the target project on the fly
The 2nd option requires additional configuration. Please have a look at the other examples to check how it's handled there, it's fine if users have to make minor edits in the scripts. All examples were set up like this.
2. apply Ludo's review
|
I replaced |
|
@ljcornel I'm waiting for your review. Please have a look. |
There was a problem hiding this comment.
I was waiting for you to address my comment #243 (comment) regarding the two use cases for the script. I saw now that you added some instructions to the readme for obtaining the project deployment, that's fine with me. Thank you
Add feature of video reconstruction with overlaid predictions on a local machine.