Made a number of changes #13
Conversation
…ng + validation data records, changed default record directory to samples/train
… first n values (useful for validation data). 2) Added early stopping based on validation loss. 3) Fixed bugs in progress printout and that batch_size was not used by data.next_batch
…bias towards driving straight, changed button push logic so that each update is a Bernoulli trial between 0/1 (rather than rounding).
…stall, hopefully got the train/val directories to be included
There was a problem hiding this comment.
Great additions! Its exciting to see contributions to the project ❤️
Can you please address the comments and then split this PR up into 3 different pull requests.
- One for the readme setup updates
- One for the updates to play.py
- Another for the validation set code
This will make it easier to track the overall history of the project and why things were done.
Thanks!
|
|
||
|
|
||
| ### Step 3: Install dependancies | ||
| sudo apt-get install dpkg-dev build-essential swig python2.7-dev libwebkitgtk-dev libjpeg-dev libtiff-dev checkinstall ubuntu-restricted-extras freeglut3 freeglut3-dev libgtk2.0-dev libsdl1.2-dev libgstreamer-plugins-base0.10-dev |
There was a problem hiding this comment.
I didn't realise this much apt-geting was required. Are freeglut3, swig, ubuntu-restricted-extras needed? they look suspect to me. I previously built opencv on my system so maybe that process made this project easier to setup for me. Another option we could explore is to build a docker container for TensorKart and then all this documentation becomes setup code that we can use instead of documentation we have to update.
also there is an extra space after libgtk2.0-dev
There was a problem hiding this comment.
If you follow this link in the references of that Instructions.txt file, you can see I just took the first step: https://nguyenph88.blogspot.com/2014/04/how-to-install-wxpython.html
I don't know if all those libraries are required, but it (finally) worked...
|
|
||
| sudo apt install python-pip | ||
| sudo pip install --upgrade pip | ||
| sudo pip install -U pip setuptools |
There was a problem hiding this comment.
do you remember why you needed setuptools?
There was a problem hiding this comment.
I think that was related to wx again... I read many times I needed to make sure pip and setuptools were updated, eg here: https://wiki.wxpython.org/How%20to%20install%20wxPython
Once I got wx working, I didn't try to reduce the number of libraries used. It is possible this step can be left out.
|
|
||
| sudo apt-get install python-tk | ||
| sudo apt-get install xboxdrv | ||
| sudo apt-get install mupen64plus |
There was a problem hiding this comment.
why not group all of the apt-get dependencies in one line
There was a problem hiding this comment.
I think there should be no reason in general. I just split it as wx dependencies, then python libraries, then emulator/controller libraries. Maybe I can label these separately?
The order may matter though, right? I also don't know if it matters whether I use apt install vs apt-get install...
There was a problem hiding this comment.
apt vs apt-get would depend on what package manager your system has
| ### Step 5: Download the Mario Kart ROM | ||
| # DL to TKproj directory | ||
| # https://www.emuparadise.me/Nintendo_64_ROMs/Mario_Kart_64_(USA)/39949-download | ||
| unzip 'Mario Kart 64 (USA).zip' |
There was a problem hiding this comment.
I'm not sure its okay to link to this location on GitHub since its probably protected under some copywrite etc.
|
|
||
| ## Terminal 2 | ||
| cd TKproj | ||
| mupen64plus --input mupen64plus-input-bot/mupen64plus-input-bot.so 'Mario Kart 64 (USA).n64' # keyboard-M to mute |
There was a problem hiding this comment.
I'd prefer to see these sections merged into my readme where I already explain this process (although in less detail than you did so this is still a great contribution).
In fact I'd like to see this whole document merged into the readme rather than adding another document and having 2 places to look for things.
In my opinion it should look like this (feel free to propose alternate ideas):
- Expand the dependencies section and move it further down in the doc
- Add a table of contents to the top of the readme
- Merge your step by step instructions with my current ones in the readme
- add your appendix section to the readme
- add the references to the readme.
What do you think?
There was a problem hiding this comment.
Most of this sounds good, I don't see why the dependencies should be at the bottom though. Presenting the info in chronological order makes sense to me.
There was a problem hiding this comment.
It depends - I imagine a lot of people will just read through the readme to understand the project and not necessarily intent to run it. Thats why I would move dependencies further down - but adding the table of contents lets people know that this information is available.
| joystick[4] = np.random.choice(a = [0,1], p = [1-p4, p4]) | ||
|
|
||
| #p5 = max(min(joystick[5], 1), 0) | ||
| #joystick[5] = np.random.choice(a = [0,1], p = [1-p5, p5]) |
There was a problem hiding this comment.
can you remove the future work comments and create issues instead please.
Otherwise adding this multiplier seems fine to me. Its interesting that Nvidia found the same thing in their paper.
Can you move the convert_to_probability code to a utility function named the same? This will remove some duplicate code and make it easier to understand and change later. It would also be nice if we didn't have to add variables to this scope and could hide all of this in the function (aka remove p2, p3, p4)
There was a problem hiding this comment.
Ok, on the comments. For using a method, I will give it a shot. As noted, I am learning Python right now, but that seems like it shouldn't be too complicated.
| int(joystick[2]), | ||
| int(joystick[3]), | ||
| int(joystick[4]), | ||
| #int(round(joystick[5])), |
There was a problem hiding this comment.
Ok, regarding the comment. Also, the comma after the last element was in your original code. Was that a typo, or does it mean something?
There was a problem hiding this comment.
mm probably by accident. I guess python doesn't care about extra commas
| ] | ||
|
|
||
| ### print to console | ||
| if (manual_override): | ||
| cprint("Manual: " + str(output), 'yellow') | ||
| else: | ||
| cprint("AI: " + str(output), 'green') | ||
| cprint("AI: " + str(output) + str(p2), 'green') |
There was a problem hiding this comment.
why are you printing this value here? Isn't it already captured in the output variable?
There was a problem hiding this comment.
Because now there are two steps before the button is "pushed". First there is some probability that the network will push it, then there is whether or not it was actually pushed (determined by pRNG). I wanted to monitor the probability that "A" was to be pushed. I agree that either that should be removed or we should monitor the same for all buttons though.
| ## Exit loop if early_stopping criteria is met | ||
| if (cur_step - best_idx) >= early_stop_steps: | ||
| stop_flag = True | ||
| break |
There was a problem hiding this comment.
I wonder if TensorFlow has a built in way to do this kind of training loop.
There was a problem hiding this comment.
I am not sure. I read this:
https://www.quora.com/How-does-one-employ-early-stopping-in-TensorFlow
Since it was already set up so that the training loop was written in python, I figured this was the easiest way. I got the impression that the tensorflow library does include methods to auto-iterate, etc but this only works for simple, pre-designed networks. That may be incorrect.
Sure, but I am not clear on how to go about splitting up these changes. Is the usual way to just restart with a new branch identical to yours, then modify it piece by piece and PR after each? This seems unwieldy. Can I select certain modifications to PR (I didn't see this option, but may have missed it)? Sorry, learning github as well... |
|
I usually try and commit each logical change as a single commit and then I can use Thanks! |
|
I did a full install of mint 18.1 to an internal drive (earlier I had only tested running it from a flash drive). There were a few odd differences that make me think those instructions need more testing, or narrowing of scope. For example, the default size of the mupen64plus window was much smaller for some reason (about 1/2 the expected size), so this should be hard coded. There were some other things about my set up that I changed as well (eg monitors), so that could be involved, but anyway the guide shouldn't depend on the exact hardware set up. However, overall it did go pretty smoothly. |
|
I wanted to look into creating a start script for the emulator that set the window position and the size but didn't get around to it. Probably some cross platform concerns there as well. |
I was able to get this all working on a fresh install of mint 18.1, it was kind of a pain with the dependencies (for wx especially), so included are step by step instructions. Once it was working, I noticed there was no way of using validation data, so made the modifications to allow for that. Right now it is set up so that train and validation are separate recordings, another approach would be to split up a single dataset, which I did not implement.
Also, I noticed the AI did not like turning strong enough although the direction of the turn was often correct. For that reason I decided to multiply the x-axis values by 2 (arbitrary, this can be tuned), which greatly improved driving performance (interestingly, this was also reported by Nvidia[1]). Another thing is that the button presses were being determined by rounding a value (roughly...) between 0-1, I thought it would be better to make this stochastic so that the value represented the probability of pushing the button when viewing a given scene. I didn't set a pRNG seed, so each race will be different (at least slightly).
NB I am new to linux, python, and tensorflow, so if you see anything strange say something. I probably did it wrong.
[1] "To remove a bias towards driving straight the training data includes a higher proportion of frames that represent road curves."
https://arxiv.org/pdf/1604.07316.pdf