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

[AdamGleave]

I've finished my review of the common files now in #89 Overall seems pretty good -- only suggested some minor tweaks.

One more general point I noticed: the documentation includes the types but we also annotate the functions. Unfortunately they were often out of sync. In general it seems hard to keep them both up to date: I'm inclined to just drop the types from the documentation, and rely on type annotations (we could probably modify Sphinx to make the output prettier, if needed). Thoughts on this? Main reason I can see against this is sometimes we might want to abbreviate the type in documentation, although I think often that's a sign that a type alias might be more appropriate.

I'm also tempted to use some autoformatters like isort for the import and black or (if that's too opinionated) yapf. I've found these have saved me a lot of time in other projects, especially if they're set to auto-run on save, so I can forget about formatting entirely. If there's interest I could make a PR to add these to the linters -- but, perhaps we do not want the machines to supplant humans entirely ;)

[araffin]

I've finished my review of the common files now in #89 Overall seems pretty good -- only suggested some minor tweaks.

Thanks =) (will try to take a look next week, currently busy with experiments on real robot...)

One more general point I noticed: the documentation includes the types but we also annotate the functions. Unfortunately they were often out of sync. In general it seems hard to keep them both up to date: I'm inclined to just drop the types from the documentation, and rely on type annotations (we could probably modify Sphinx to make the output prettier, if needed). Thoughts on this? Main reason I can see against this is sometimes we might want to abbreviate the type in documentation, although I think often that's a sign that a type alias might be more appropriate.

Yes, there is an issue about that #10 (that you already commented a while ago ^^)

I'm also tempted to use some autoformatters like isort for the import and black or (if that's too opinionated) yapf. I've found these have saved me a lot of time in other projects, especially if they're set to auto-run on save, so I can forget about formatting entirely.

In fact, I've seen that you have been using black and I wanted to ask you your feelings about that. I used it recently on some projects and I've got mixed feelings. So overall, it was nice to format everything automatically, but sometimes it formats it in a way I don't like.
I don't know how much you can tweak black... (need to read more doc), but if we can tweak it enough, then I would be for ;)

[araffin]

@hill-a as discussed privately, I'll assign you for SAC and TD3 review ;)

[AdamGleave]

Yes, there is an issue about that #10 (that you already commented a while ago ^^)
Ah I'd forgotten about that, OK!

In fact, I've seen that you have been using black and I wanted to ask you your feelings about that. I used it recently on some projects and I've got mixed feelings. So overall, it was nice to format everything automatically, but sometimes it formats it in a way I don't like.
I don't know how much you can tweak black... (need to read more doc), but if we can tweak it enough, then I would be for ;)

Black's philosophy is "any colour you like [so long as it's black]", so customizability is not it's strong suit. I do also object to the formatting decisions made by black sometimes, but on balance prefer it compared to no auto-formatting. I think yapf is quite customizable but I've never used it.

[araffin]

After some quick trials, yapf corresponds more to the existing codestyle.
You can try online: https://yapf.now.sh/

It is not perfect when wrapping the lines but it already does a good job (and I prefer the codestyle vs the one imposed by black).

[araffin]

After some more testing, I would go for black (maybe in combination with bug bear).
I was not able to define a style with yapf that fits me...

[araffin]

As the TODO-list for v1.0 is almost complete, we should finish the code review.
@hill-a do you have time to review the off-policy algorithms (SAC/TD3)?
Otherwise, does anyone else can do it? (maybe @partiallytyped ?)

Regarding the documentation, i think it should be ok now? (see #166 )

[AdamGleave]

Regarding the documentation, i think it should be ok now? (see #166 )

Yep, I think the documentation is in good shape 👍