Review of Existing Code

[AdamGleave]

@araffin has done a great job creating this version of the library but has done it mostly solo, so much of the code has never been reviewed.

I suggest the other maintainers each take responsibility for reviewing a portion of the code. Rather than doing a traditional code review, since it's already committed I suggest we just make a PR with any changes we think should take place, or raise an issue for non-trivial proposals.

I think the priority is code that is used in multiple algorithms and/or defines the public API and which is new. This includes:

• common/base_class.py
• common/distributions.py
• common/policies.py
• common/type_aliases.py

Next would be the individual algorithms:

• A2C
• PPO
• SAC
• TD3

Also new parts of the documentation could use re-reading/confirming:

• Documentation

If this sounds like a good idea to others, then perhaps we can each claim a few entries on the above list and then start a PR for it? Also feel free to edit the post to break it up differently or to add files I've missed.

[AdamGleave]

Some self-review from @araffin (lightly edited, mistakes my own):

• A2C:
  • Imports policy from PPO, could move it to common e.g. as OnlineActorCriticPolicy.
  • Also derives from PPO, so again introducing a base class e.g. OnlineAlgorithm could help.
• TD3 and SAC should share part of their policy.
• TD3: policy does not handle action noise so if saved independently of the RL algorithm it will only be a deterministic policy.

[Miffyli]

I can do A2C and PPO review (core algorithms + files they use), along with updating them to share the code more elegantly as pointed above (OnlineActorCriticPolicy etc).

Edit: I could also do tests / fixes for Windows 10. I know this is not the main platform for this, but I would like to see some support for it. So far it passes almost all tests. Update: Error was in my end. Things run smoothly on Windows 10.

[araffin]

@Miffyli nice =) (apparently, we can add Windows and mac os thanks to github ci (to be tested))
i think some os.path.join are missing, no?

i will ping @hill-a privately ;) (he needs some motivation but he can do things and his input is usually valuable)
maybe an additional "auto-review": i did not use "_" prefix for all private attributes.

[AdamGleave]

I'm happy to review common/base_class.py, common/distributions.py, common/policies.py and common/type_aliases.py although may not have time until after NeurIPS deadline.

[araffin]

Mentioned somewhere: tmp_path should be used everywhere in the tests instead of creating custom path