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

Refactoring to use contexts managers, list comprehensions when more idiomatic, and minor renaming to help reader clarity #2335

Merged
merged 3 commits into from Jun 22, 2020

Conversation

QuentinDuval
Copy link
Contributor

No description provided.

…diomatic, and minor renaming to help reader clarity.
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.

LGTM, thanks a lot!

def __init__(self, x):
self.x = x
def __init__(self, video_paths: List[str]):
self.video_paths = video_paths
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is fine because this is a private class, but in general renaming methods is a BC-breaking change

frame = av.VideoFrame.from_ndarray(img, format="rgb24")
frame.pict_type = "NONE"
for packet in stream.encode(frame):
with av.open(filename, mode="w") as container:
Copy link
Member

Choose a reason for hiding this comment

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

thanks! PyAV only added support for using open as a context-manager in 6.2.0, which was after we released this first version.
This clean-up is much appreciated!

@fmassa
Copy link
Member

fmassa commented Jun 22, 2020

Lint is failing in https://app.circleci.com/pipelines/github/pytorch/vision/2948/workflows/a769a571-a0d6-46f9-9de8-a1ed3c4be2b0/jobs/162851/steps with

./torchvision/datasets/video_utils.py:53:1: W293 blank line contains whitespace

can you fix it?

@QuentinDuval
Copy link
Contributor Author

Lint is failing in https://app.circleci.com/pipelines/github/pytorch/vision/2948/workflows/a769a571-a0d6-46f9-9de8-a1ed3c4be2b0/jobs/162851/steps with

./torchvision/datasets/video_utils.py:53:1: W293 blank line contains whitespace

can you fix it?

Indeed! It should be fixed at the latest commit.

@fmassa
Copy link
Member

fmassa commented Jun 22, 2020

There is a conflict now that I merged your other PR. Could you address it please?

@fmassa fmassa merged commit 42aa9b2 into master Jun 22, 2020
@fmassa fmassa deleted the video_refactoring branch June 22, 2020 14:59
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