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

Turn on ruff linter, fix final 7 bugs #78

Merged
merged 6 commits into from
Jul 24, 2024
Merged

Turn on ruff linter, fix final 7 bugs #78

merged 6 commits into from
Jul 24, 2024

Conversation

sritchie
Copy link
Contributor

@sritchie sritchie commented Jul 23, 2024

This PR turns on the linter; we can't merge until we address the final linter bugs.

  • ruff is enabled as a GitHub Action, so that we see errors in the PRs in github format
  • ruff locally is added (with the same version) to .pre-commit-config.yml

@mirkoklukas , would you mind taking a look at the line that I pointed out in the PR?

@@ -172,9 +172,6 @@ def video_input_from_mp4(
downsize=1,
reverse_color_channel=False,
):
if times is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mirkoklukas , T wasn't available, so we can't do this calculation here; we could pass times back OUT of load_video_to_numpy, but that's a bigger change.

Can you let us know if the below change, where we swap your fps for info.fps, is valid?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think adding T = info.timesteps before the None check should do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Boom, fixed!

@sritchie sritchie merged commit 0256164 into main Jul 24, 2024
2 checks passed
@sritchie sritchie deleted the sritchie/lint_bugs branch July 24, 2024 14:42
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