-
Notifications
You must be signed in to change notification settings - Fork 4.4k
pull/1294 from has-taiar #1310
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
pull/1294 from has-taiar #1310
Conversation
This is on top of this PR. Thanks for the contribution from #1294. |
docs/Installation.md
Outdated
width="500" border="10" /> | ||
</p> | ||
|
||
## Windows installation |
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.
Should keep the header "Windows Users"
docs/Installation.md
Outdated
guide](Installation-Windows.md) to setting up your env. For Mac and Linux, | ||
continue with this guide. | ||
|
||
## Clone the ML-Agents Toolkit Repository |
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.
This header should be renamed to "## Mac and Unix Users", immediately follows by a sub-heading "### Clone the ML-Agents Toolkit Repository"
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.
Further, below:
## Install Python and mlagents Package
-->### Install Python and mlagents Package
### NOTES
-->**Notes:**
(bold instead of header)- Delete
### Mac and Unix Users
header
These suggested are following the thinking that the intention here is to have three top-level sections:
- Windows Users
- Mac and Unix Users
- Docker Installation
…, added the package explanation to the Windows installation doc
Hi @mmattar , I've resolved all of your comments. |
docs/Installation-Windows.md
Outdated
To use the ML-Agents toolkit, you install Python and the required Python | ||
packages as outlined below. This guide also covers how set up GPU-based training | ||
(for advanced users). GPU-based training is not required for the v0.4 release of | ||
(for advanced users). GPU-based training is not required for the v0.6 release of |
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.
On second thought, can we just remove v0.6 and say "for the current release"
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.
More appropriately .... "GPU-based training is not currently required for the ML-Agents toolkit."
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.
Fixed.
docs/Installation-Windows.md
Outdated
check Nvidia's page [here](https://developer.nvidia.com/cuda-gpus). | ||
|
||
As of the ML-Agents toolkit v0.4, only CUDA v9.0 and cuDNN v7.0.5 is supported. | ||
As of the ML-Agents toolkit v0.6, only CUDA v9.0 and cuDNN v7.0.5 is supported. |
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.
similar to above - remove the v0.6 as we'll always need to uptick
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.
Fixed both places.
docs/Installation-Windows.md
Outdated
|
||
The `UnitySDK` subdirectory contains the Unity Assets to add to your projects. | ||
It also contains many [example environments](Learning-Environment-Examples.md) | ||
that can be used to help get you familiar with Unity. |
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.
"that can be used to help get you familiar with Unity." --> "to help you get started."
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.
Fixed, I also fixed the exact same wording in the Installation.md doc.
If you don't want to use Git, you can always directly download all the files | ||
[here](https://github.com/Unity-Technologies/ml-agents/archive/master.zip). | ||
|
||
The `UnitySDK` subdirectory contains the Unity Assets to add to your projects. |
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.
Why are we referring to the SDK as assets?
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.
I copied over the same sentences in the Installation doc to this Installation windows doc. Also they can be considered as Assets since they are something we can put in the Asset store. (And are compatible with the Asset usage, while the python package isn't).
@mmattar I've resolved all of the comments. |
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.
@xiaomaogy - one minor fix - otherwise 👍 - Thanks.
docs/Installation-Windows.md
Outdated
check Nvidia's page [here](https://developer.nvidia.com/cuda-gpus). | ||
|
||
As of the ML-Agents toolkit v0.6, only CUDA v9.0 and cuDNN v7.0.5 is supported. | ||
Currently for ML-Agents toolkit, only CUDA v9.0 and cuDNN v7.0.5 is supported. |
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.
Currently for --> Currently for the
No description provided.