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 notes, a few NITs #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added notes, a few NITs #1

wants to merge 1 commit into from

Conversation

krshrimali
Copy link
Collaborator

The code overall looks good. Great work! I've added comments in the code and have done a few changes.

  • Please note that the comments I made start with Note: so that you can easily spot. The lines that follow comments starting with Note: are also made by me (there maybe some exceptions).
  • I don't know how much are you concerned with the quality of code for development or production purposes, but the suggestions I made are inclined to make the code readable, understandable and deployable for general use later on.
  • You might want to have an Argument() class which takes your command line arguments and uses either the default argument (in case the argument isn't passed) or the command line argument (if passed). It comes handy for most of the DL projects, since you experiment with different hyper-params.
  • Having whitespaces or extra tabs is usually avoidable, and if you are familiar with vim, you can simply do / $ in visual mode. I have removed any extra whitespaces or tabs, you can take a look at them in the changes.
  • I believe the base class files Environment.cpp and A2CAgent.cpp are inherited (I maybe wrong), and I hope they perform fine to your expectations.

You can surely ignore these suggestions/comments as you should do what you feel is the best :)

cc: @vittorione94

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.

1 participant