-
Notifications
You must be signed in to change notification settings - Fork 112
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
Code Improvements #4
Conversation
Those improvements are neat! Bumping |
LGTM |
Thank you for your contribution to modernize the codebase! I'll have a closer look and do the tests during the weekend before the merge, but will leave some few comments from me in the meantime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blaisewf could you check my comments ahd share your thoughts?
Hey there, side question. Have you guys considered running benchmarks on different GPUs to check the performance (CUDA/CPU etc) and VRAM used? (if possible considering also consumer grade GPUs and stuff like the Real Time Factor, inference speed or time etc) to provide in the That could be really useful |
We'll also consider adding the speed benchmark to |
All requested changes should be ready by now! |
Merged with fixes that ensure backward compatibility and few extras (type hint and docstrings). Thanks again for your contribution! |
Hi, I've made some general changes to improve the experience, especially developing since the repository was a bit messy.
As I have modified several files, let me know if there are any bugs, or you want to revert something, it should work fine. Take your time and feel free to change anything you want without my consent 😄
Summary of Changes: