Feature/New acme adders and tests. #276
Conversation
There was a problem hiding this comment.
Great work @KaleabTessera 😄 Thanks for the massive effort 🔥 Did you spot-check that the different properties like recurrent, state-based, centralised still work? I think I checked most of them though. So they should probably still work.
|
Thanks @DriesSmit ! I wrote tests for feedforward and recurrent systems, so they should work/run. I will quickly add smoke tests for state based, centralized and networked archs. |
There was a problem hiding this comment.
Great work! Thanks @KaleabTessera!! 🔥 Huge effort! 💪
Please just see my few comments. All minor.
| @@ -83,7 +83,9 @@ def __init__( | |||
| client: reverb.Client, | |||
| n_step: int, | |||
| discount: float, | |||
| *, | |||
There was a problem hiding this comment.
So I saw online: "The * indicates the end of the positional arguments. Every argument after that can only be specified by keyword." Just wondering why we are being explicit about it?
There was a problem hiding this comment.
My understanding of *, is that it is useful when they are multiple arguments and you want to split "common" arguments from "less common" arguments.
The arguments before *, can be provided positionally and that usually makes sense for parms that are common to a function call. The params after *, , are less common and would be difficult to keep track of their positional spot since there are a lot of them and so are keyword args.
| @@ -469,15 +482,15 @@ def __init__( | |||
| ) | |||
|
|
|||
| # Forward pass that calculates loss. | |||
| def _forward(self, inputs: Any) -> None: | |||
| def _forward(self, inputs: reverb.ReplaySample) -> None: | |||
| """Trainer forward pass | |||
|
|
|||
| Args: | |||
| inputs (Any): input data from the data table (transitions) | |||
| """ | |||
|
|
|||
| # TODO: Update this forward function to work like MAD4PG | |||
There was a problem hiding this comment.
Do we want to keep this TODO?
There was a problem hiding this comment.
Not sure, @DriesSmit do we still need this?
| try: | ||
| import pyspiel # type: ignore | ||
| from open_spiel.python import rl_environment # type: ignore | ||
| except ModuleNotFoundError: |
There was a problem hiding this comment.
Nice, not sure if this is the reason, but we should perhaps consider this for other environments as well for when they haven't been installed as dependencies.
There was a problem hiding this comment.
I did a quick pass and did this for the other environments.
What?
Why?
How?
Extra