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

added param typing to walk function and updated the walk function to take in num_steps as a list #52

Merged
merged 10 commits into from
Oct 3, 2022

Conversation

danielpatrickhug
Copy link
Contributor

Hello, @nateraw
issue reference: #41

  • this pull request changes the function parameter num_steps from an int to a list.
  • the code has been tested on colab as my local computer does not have a gpu
  • tested w/ bools, resume, do_loop, and passing in an int instead of a list for num_steps.

Copy link
Owner

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your contribution ❤️

I would suggest leaving the int type for num steps so we have backwards compatibility. type as Union[int, List[int]]. Leave default as 5 (I don't like list defaults, they can do wonky things in Python).

resume=False,
batch_size=1,
frame_filename_ext='.png',
prompts:list = ["blueberry spaghetti", "strawberry spaghetti"],
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the type hints here! But can we do List[str]/List[int] for list types? Using from typing import List..., etc.

frame_filename_ext='.png',
prompts:list = ["blueberry spaghetti", "strawberry spaghetti"],
seeds:list = [42, 123],
num_steps:list=[5],
Copy link
Owner

Choose a reason for hiding this comment

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

Leave default here as int. type it as Union[int, List[int]].

Copy link
Owner

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

Some minor suggestions/comments. I am doing some traveling today so might not get to merging this for a day or so, btw.

):
"""Generate video frames/a video given a list of prompts and seeds.

Args:
prompts (List[str], optional): List of . Defaults to ["blueberry spaghetti", "strawberry spaghetti"].
seeds (List[int], optional): List of random seeds corresponding to given prompts.
num_steps (int, optional): Number of steps to walk. Increase this value to 60-200 for good results. Defaults to 5.
num_steps (list, optional): list of number of steps to walk during each interpolation step. The size of num_steps should be len(prompts)-1. Increase this value to 60-200 for good results. Defaults to [5].
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
num_steps (list, optional): list of number of steps to walk during each interpolation step. The size of num_steps should be len(prompts)-1. Increase this value to 60-200 for good results. Defaults to [5].
num_steps (Union[int, List[int]], optional): Number of steps to walk during each interpolation step. If int is provided, use same number of steps between each prompt. If a list is provided, the size of `num_steps` should be `len(prompts) - 1`. Increase values to 60-200 for good results. Defaults to 5.

Copy link
Owner

Choose a reason for hiding this comment

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

formatting might need to be updated here.

resume=False,
batch_size=1,
frame_filename_ext='.png',
prompts:List[str] = ["blueberry spaghetti", "strawberry spaghetti"],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
prompts:List[str] = ["blueberry spaghetti", "strawberry spaghetti"],
prompts: List[str] = ["blueberry spaghetti", "strawberry spaghetti"],

If you can, please apply this spacing to all args 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Or if you don't want to go back and forth on my style nit-picks I can do it myself in separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! no, I agree with those changes. thank you for bringing them up. it would be good to add a Makefile to run style checks and linters.

Copy link
Owner

Choose a reason for hiding this comment

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

Tracking that with #54


frame_index = 0
for prompt, seed in zip(prompts, seeds):
total_frame_count = sum(num_steps)
Copy link
Owner

Choose a reason for hiding this comment

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

Very clean!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you.

@nateraw
Copy link
Owner

nateraw commented Sep 23, 2022

@danielpatrickhug ready for review again?

@danielpatrickhug
Copy link
Contributor Author

danielpatrickhug commented Sep 24, 2022

@nateraw Hey, yes, it should be good to go. please let me know if you have any other suggestions :)

@nateraw
Copy link
Owner

nateraw commented Sep 26, 2022

cool - apologies if it takes me a few days to get this merged...will have to resolve conflict + give it a test drive and I have a few other things I'm juggling. From first glance it looks like its ready to go though :)

@nateraw nateraw merged commit 402bdd9 into nateraw:main Oct 3, 2022
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