Repository navigation
Conversation
|
Hi all, just following up on this PR. I believe it's ready for review, but please let me know if there's anything else I should address. Thanks for your time! |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The ARS change itself isn't covered here. The added tests are for SAC and TD3, so make_policy could return the wrong parameter key for behavior/eval and CI wouldn't catch it. Can you add a small ARSBuilder.make_policy test for evaluation=False -> policy and True -> eval?
Checks that make_policy returns the behavior params key for evaluation=False and the eval params key for evaluation=True, and that the network is passed through unchanged.
|
Thanks for the review! I added acme/agents/jax/ars/builder_test.py, which checks that make_policy returns the policy params key for evaluation=False and eval for evaluation=True, and that the network is passed through unchanged. Happy to move the SAC/TD3 learner tests into a separate PR if you'd prefer this one to cover only ARS. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked 180727c. The new ARSBuilder test now covers the exact behavior I asked about: behavior mode selects the policy params key, evaluation mode selects the eval key, and the network object is passed through unchanged. That resolves my blocker. The SAC/TD3 learner tests are unrelated and could be split for scope, but that is non-blocking from my side.
|
Hi, |
2 participants
ARSBuilder was the only builder in acme/agents/jax that did not implement make_policy. The base class raises NotImplementedError for it, with a comment noting it should be made abstract once all agents implement it -- ARS was the remaining blocker.
The implementation delegates to make_policy_network, which already exists in ars/networks.py and handles the behavior/eval distinction via its eval_mode flag. The only other change is adding the missing import for ars/networks.