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

[videoAPI] minor example and documentation changes #2821

Merged
merged 8 commits into from
Oct 30, 2020

Conversation

bjuncek
Copy link
Contributor

@bjuncek bjuncek commented Oct 15, 2020

There were still a few points where documentation should have been clarified, and
example notebook needed to change the return type. Should be cleared up now.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 16, 2020

@bjuncek on Colab running the notebook on ('1.8.0.dev20201015+cpu', '0.8.0.dev20201015+cpu') from pypi, I have the following error:

/usr/local/lib/python3.6/dist-packages/torchvision/io/__init__.py in __init__(self, path, stream)
     88     def __init__(self, path, stream="video"):
     89         if not _has_video_opt():
---> 90             raise RuntimeError("Not compiled with video_reader support")
     91         self._c = torch.classes.torchvision.Video(path, stream)
     92 

RuntimeError: Not compiled with video_reader support

What do you think to add a message about to install manually ffmpeg and build torchvision from tag to have the feature ?
Idea is to guide the user a bit more...

  • Another point, probably, a more generic question. In Colab you are make_dataset method which is not documented at all.
    Maybe, it worth adding a docstring.

  • it would be nice to add an image of frames from the batch in the end of the notebook.

import matplotlib.pylab as plt
%matplotlib inline

plt.figure(figsize=(12, 12))
for i in range(16):
  plt.subplot(4, 4, i + 1)
  plt.imshow(b["video"][11, i, ...].permute(1, 2, 0))
  plt.axis("off")

@bjuncek
Copy link
Contributor Author

bjuncek commented Oct 19, 2020

Hi @vfdev-5

What do you think to add a message about to install manually ffmpeg and build torchvision from tag to have the feature?

Since there is a warning there already, might as well. Added in subsequent commit.

Another point, probably, a more generic question. In Colab you are make_dataset method which is not documented at all.

Isn't it documented in the DatasetFolder?

it would be nice to add an image of frames from the batch in the end of the notebook.

done :)

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks @bjuncek

Failing Travis CI is unrelated.

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #2821 into master will decrease coverage by 1.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2821      +/-   ##
==========================================
- Coverage   73.35%   72.27%   -1.09%     
==========================================
  Files          99       99              
  Lines        8785     8785              
  Branches     1387     1387              
==========================================
- Hits         6444     6349      -95     
- Misses       1916     1998      +82     
- Partials      425      438      +13     
Impacted Files Coverage Δ
torchvision/io/__init__.py 89.65% <0.00%> (ø)
torchvision/ops/_register_onnx_ops.py 48.78% <0.00%> (-36.59%) ⬇️
torchvision/models/detection/transform.py 78.33% <0.00%> (-17.23%) ⬇️
torchvision/ops/poolers.py 85.29% <0.00%> (-11.77%) ⬇️
torchvision/ops/boxes.py 87.35% <0.00%> (-8.05%) ⬇️
torchvision/models/detection/roi_heads.py 77.23% <0.00%> (-5.11%) ⬇️
torchvision/models/detection/rpn.py 90.18% <0.00%> (-3.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99bf4c0...8396963. Read the comment docs.

Copy link
Member

@fmassa fmassa left a 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 have one blocking comment (the doc is now broken), otherwise the other comments are minor.

torchvision/io/__init__.py Outdated Show resolved Hide resolved
torchvision/io/__init__.py Outdated Show resolved Hide resolved
examples/python/video_api.ipynb Outdated Show resolved Hide resolved
torchvision/io/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit 36daee3 into pytorch:master Oct 30, 2020
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Modified the example to conform with the new DICT return.

* adding better stream documentation to examples

* Clearing up some documentation as a result of a feedback

* Formatting mostly

* Addressing Victor's comments.

* addressing fmassas comments

* remove unnecessary tab

Co-authored-by: Bruno Korbar <[email protected]>
Co-authored-by: vfdev <[email protected]>
vfdev-5 added a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Modified the example to conform with the new DICT return.

* adding better stream documentation to examples

* Clearing up some documentation as a result of a feedback

* Formatting mostly

* Addressing Victor's comments.

* addressing fmassas comments

* remove unnecessary tab

Co-authored-by: Bruno Korbar <[email protected]>
Co-authored-by: vfdev <[email protected]>
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.

3 participants