Miscellaneous GAIL improvements and refactoring #133
Conversation
qxcv
changed the title
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 87.14% 87.13% -0.01%
==========================================
Files 60 60
Lines 4332 4329 -3
==========================================
- Hits 3775 3772 -3
Misses 557 557
Continue to review full report at Codecov.
|
| mean_stats = { | ||
| k: np.mean(v) for k, v in stat_dict_accum.items() | ||
| } | ||
| return mean_stats |
There was a problem hiding this comment.
Do you have any code that uses the mean_stats return value? I think that in a later commit I can cleanly merge all of the mean accumulating logic here with the mean-accumulating custom logger from #135.
The mean-accumulating logger gets to write the means to an SBLogger (whereas the means aren't logged here), so I'm wondering if you think we will still need to return mean_stats later on.
There was a problem hiding this comment.
I do, but I won't need it once #135 is removed. It's fine to take out.
There was a problem hiding this comment.
(I just added a comment saying you can remove it; I'll let you do the honours when you merge #135)
| log_fmts = [ | ||
| make_output_format(s, disc_log_dir) for s in log_fmt_strs | ||
| ] | ||
| self._disc_logger = Logger(disc_log_dir, log_fmts) |
There was a problem hiding this comment.
FYI, When I merge with #135 later, probably won't use this second logger. Instead will use a "discrim" context.
This is fine by me for this PR though.
Co-Authored-By: Steven H. Wang <[email protected]>
Co-Authored-By: Steven H. Wang <[email protected]>
qxcv
force-pushed
the
gail-reward-net-refactor
branch
from
3e49a29 to
e4be624
Compare
src/imitation/rewards/discrim_net.py
Outdated
| construction of the discriminator network, and a `tf.Tensor` | ||
| representing the desired discriminator logits. | ||
| build_discrim_net_kwargs: optional extra keyword arguments for | ||
| `build_discrim_net()`. |
There was a problem hiding this comment.
Now that we have the build_discrim_net_kwargs pattern, could we make build_mlp_discrim_net into a function rather than a class?
Not sure if pytype would be happy with Callable + arbitrary kwargs. (It looks like newer versions of Python will have a more precise Protocol type for defining callable types: https://stackoverflow.com/a/57840786/1091722)
|
Done! Sorry for the delay, I forgot that I still had a change to make. |
This PR includes a set of changes that make GAIL more flexible & easier to use. The DAgger PR (#128) and the BC PR (#125) should be merged before this one, since the branch I'm pulling from is based on both of those. New changes here:
make_summary_writerwrites its logs & makesAdversarialTrainerresponsible for passing in that directory.make_summary_writerintoinit_trainerso that the resulting output directory can be used for logs, and not just TB summaries.build_methods fromDiscrimNet(build_{train_reward,test_reward,summaries,disc_loss}, IIRC) and replaces them with onebuild_graphmethod. Also merges the twobuild_methods inAdversarialTrainer.LoggertoAdversarialTrainerthat records discriminator stats at each update. At the moment this only works for GAIL; the extension to AIRL is straightforward, but I don't have time to test it manually, so I haven't done it myself.Originally I was going to refactor
DiscrimNetentirely so that it passes around Keras models instead of construction functions and kwargs, but I don't have time to do so at the moment (I've added it to the wishlist in #31).