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

Add mountain car reward heatmap and reward-vs-time plotters #177

Merged
merged 22 commits into from
Apr 7, 2020
Merged

Conversation

shwang
Copy link
Member

@shwang shwang commented Mar 19, 2020

Putting the Heatmap / reward vs time plotting code from my ipynb into a python file.

Not sure if experiments/ is the best place to put this. Maybe imitation.scripts is better since the user might want to import these plotting functions into another file.

While translating the code from one format to another, I found some bugs, related yet again to VecNormalize. In this case it seems like my reward functions might not have actually received normalized inputs.

So I'd expect heatmaps to look different upon a new run.

@shwang shwang requested a review from AdamGleave March 19, 2020 03:33
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #177 into master will increase coverage by 2.25%.
The diff coverage is 98.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   85.63%   87.89%   +2.25%     
==========================================
  Files          64       67       +3     
  Lines        4519     4683     +164     
==========================================
+ Hits         3870     4116     +246     
+ Misses        649      567      -82     
Impacted Files Coverage Δ
src/imitation/analysis/mountain_car_plots.py 98.07% <98.07%> (ø)
src/imitation/rewards/common.py 100.00% <100.00%> (ø)
src/imitation/rewards/serialize.py 100.00% <100.00%> (ø)
src/imitation/util/reward_wrapper.py 89.58% <100.00%> (-0.22%) ⬇️
src/imitation/util/rollout.py 95.70% <100.00%> (ø)
tests/test_density_baselines.py 100.00% <100.00%> (ø)
tests/test_mountain_car_plots.py 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96d9f8d...19c798e. Read the comment docs.

Copy link
Member

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good. One change I'd suggest in your style is using explicit matplotlib Figure and Axes objects, rather than depending on the global state in matplotlib. This is more flexible and less error prone. Otherwise fairly minor comments.

experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
experiments/mountain_car_plots.py Outdated Show resolved Hide resolved
src/imitation/util/reward_wrapper.py Outdated Show resolved Hide resolved
@shwang
Copy link
Member Author

shwang commented Mar 24, 2020

Thanks for the review. I've unsuccessfully attempted before to figure out how to gracefully use the OOP interface to matplotlib, so it was good to learn a cleaner way to manage these plots.

It's a pain for me to manually check whether plots are working so I'm going to write a smoke test of some sort. Ask for another round of review after then.

@shwang
Copy link
Member Author

shwang commented Mar 28, 2020

I moved some reward-related helper functions and util.reward_wrapper.RewardFn into a new submodule imitation.rewards.common.

To make the functions importable and testable, I moved mountain_car_plots.py from experiments/ to a new submodule imitation.analysis.mountain_car_plots. (Also considered imitation.scripts, but this isn't really a script right now, more like a collection of plotting tools.)

This should be ready for another round of review now.

@shwang
Copy link
Member Author

shwang commented Mar 29, 2020

Somehow, this branch is set off the Sacred race condition that is fixed by IDSIA/sacred#473 4 out of 5 times I reran tests.

Maybe this is bad luck on my part (in one of the reruns, train_experts.sh was the breaking script, in another rerun train_experts.sh was fine but transfer_learn...sh was broken.); maybe this is because CircleCI somehow changed.

@shwang
Copy link
Member Author

shwang commented Mar 29, 2020

I think it would be really helpful for me if Sacred configs were picklable. This would allow us to upgrade to a version of Sacred that doesn't run into the FileStorageObserver race condition when I'm running local experiments, and let me use the same version of Sacred that is used by the Minecraft project.

Asked on IDSIA/sacred#508 if all they need is just dropping in pyrsistent.PMap.

@shwang shwang requested a review from AdamGleave March 29, 2020 01:02
Copy link
Member

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, new OO-style for matplotlib much easier to understand, and +1 for tests.

There's a few minor changes I've suggested but coding as approve since no need for me to review again.

src/imitation/analysis/mountain_car_plots.py Outdated Show resolved Hide resolved
src/imitation/analysis/mountain_car_plots.py Outdated Show resolved Hide resolved
src/imitation/analysis/mountain_car_plots.py Outdated Show resolved Hide resolved
src/imitation/rewards/common.py Show resolved Hide resolved
shwang and others added 2 commits April 7, 2020 15:06
Co-Authored-By: Adam Gleave <adam@gleave.me>
Co-Authored-By: Adam Gleave <adam@gleave.me>
@shwang shwang merged commit d4c2806 into master Apr 7, 2020
@shwang shwang deleted the mc_plots branch April 7, 2020 23:20
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.

2 participants