upgrade week08 #453
upgrade week08 #453
Conversation
MichaelSolotky
commented
Aug 9, 2020
MichaelSolotky
commented
- Basically made it look more similar to the corresponding PyTorch version (markdown cells were mostly copied from it)
- Changed a bit the picture from the pytorch version to fit TensorFlow order of dimensions in image tensors
|
Check out this pull request on Review Jupyter notebook visual diffs & provide feedback on notebooks. Powered by ReviewNB |
| "from keras.layers import Conv2D, Dense, Flatten\n", | ||
| "\n", | ||
| "\n", | ||
| "class FeedforwardAgent:\n", |
There was a problem hiding this comment.
Why is FeedforwardAgent gone now?
There was a problem hiding this comment.
There's no such architecture in pytorch version, so I thought it's excess here
| @@ -352,9 +372,54 @@ | |||
| "cell_type": "markdown", | |||
| "metadata": {}, | |||
| "source": [ | |||
| "# Actor-critic\n", | |||
| "# Actor-critic objective\n", | |||
There was a problem hiding this comment.
Did this come from the PyTorch version?
| @@ -78,7 +82,6 @@ | |||
| " return env\n", | |||
There was a problem hiding this comment.
Above this line, the PyTorch version crops 15px from the top of the image by default. In this version, the default is 5. I'm not sure if that actually matters, but this is non-trivial difference between the two notebooks.
There was a problem hiding this comment.
A few more comments.
Have you tested the updated notebook end-to-end?
Yep, I have. Well, the testing was like to check whether learning curves grow and show similar behavior to the PyTorch version, that nothing crashes with restarting and running everything again.
There was a problem hiding this comment.
Above this line, the PyTorch version crops 15px from the top of the image by default. In this version, the default is 5. I'm not sure if that actually matters, but this is non-trivial difference between the two notebooks.
Yeah, it looks like it came from the previous TensorFlow version. I'll unify them if you want
There was a problem hiding this comment.
I'll unify them if you want
Please do.
| "\n", | ||
| " if i % 100 == 0:\n", | ||
| " rewards_history.append(np.mean(evaluate(agent, env, n_games=1)))\n", | ||
| " clear_output(True)\n", | ||
| " plt.plot(rewards_history, label='rewards')\n", | ||
| " plt.plot(rewards_history, label='rewards', linewidth=3)\n", |
There was a problem hiding this comment.
Are the increased linewidth and fontsize really necessary?
There was a problem hiding this comment.
It's not necessary, it just looks better IMHO. It's sometimes more difficult to look at thin lines if one is distant from a display.
