-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RLlib]: Fix FQE Policy call #26671
[RLlib]: Fix FQE Policy call #26671
Conversation
Signed-off-by: rapotdar <[email protected]>
Signed-off-by: rapotdar <[email protected]>
Signed-off-by: rapotdar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for not having a unit test here?
@@ -162,6 +166,44 @@ def test_ope_in_algo(self): | |||
print(*list(std_est.items()), sep="\n") | |||
print("\n\n\n") | |||
|
|||
def test_fqe_model(self): | |||
# Miscellaneous tests for FQETorchModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write a docstring on what this function is testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are added. Reviewed again and approved @richardliaw
Signed-off-by: rapotdar <[email protected]>
Signed-off-by: Xiaowei Jiang <[email protected]>
Signed-off-by: Stefan van der Kleij <[email protected]>
Why are these changes needed?
Hotfix to make OPE work for both PolicyV2 and DQNTorchPolicy (Policyv1)
Checks
scripts/format.sh
to lint the changes in this PR.