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

Code Improvements #4

Merged
merged 16 commits into from
Jul 21, 2024
Merged

Code Improvements #4

merged 16 commits into from
Jul 21, 2024

Conversation

blaisewf
Copy link
Contributor

@blaisewf blaisewf commented Jul 18, 2024

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:

  • Converted all string formatting to use f-strings for improved readability and performance.
  • Implemented minor changes to the discriminator.
  • Enhanced readability and efficiency of the model code.
  • Made further improvements for better clarity and presentation.
  • Added new tests folder and alias.
  • General code formatter with Black.

@realcoloride
Copy link

Those improvements are neat! Bumping

@egorsmkv
Copy link

LGTM

@L0SG
Copy link
Collaborator

L0SG commented Jul 18, 2024

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.

Copy link
Collaborator

@L0SG L0SG left a 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?

alias/cuda/activation1d.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@realcoloride
Copy link

realcoloride commented Jul 18, 2024

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 README.md?

That could be really useful

@L0SG
Copy link
Collaborator

L0SG commented Jul 18, 2024

We'll also consider adding the speed benchmark to README.md, thanks @realcoloride for the suggestion 😄

@blaisewf
Copy link
Contributor Author

All requested changes should be ready by now!

@L0SG L0SG merged commit fdc800e into NVIDIA:main Jul 21, 2024
@L0SG
Copy link
Collaborator

L0SG commented Jul 21, 2024

Merged with fixes that ensure backward compatibility and few extras (type hint and docstrings). Thanks again for your contribution!

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.

4 participants