-
Notifications
You must be signed in to change notification settings - Fork 246
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
Fix benchmark configs #665
Conversation
(y) Will review tomorrow. |
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.
Thanks for the review @Rocamonde ! |
Unfortunately I don't really have this file! You should ping @taufeeque9 who ran the experiments & gave me the JSON's. |
All right then please @taufeeque9 please add the files to this PR when you get around to it. |
2f3b1de
to
8c67937
Compare
Codecov Report
@@ Coverage Diff @@
## master #665 +/- ##
==========================================
- Coverage 97.57% 97.51% -0.06%
==========================================
Files 89 87 -2
Lines 8602 8573 -29
==========================================
- Hits 8393 8360 -33
- Misses 209 213 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This change just made some error messages go away indicating the missing imitation.algorithms.dagger.ExponentialBetaSchedule but it did not fix the root cause.
092caab
to
c8e55cb
Compare
@Rocamonde I think this is ready for another review now that Taufeeque has added the missing beta scheduler |
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, just some minor comments. please do take a look before merging
Description
imitation.algorithms.dagger.ExponentialBetaSchedule
(@Rocamonde could you do this?)More details in #664.
Fixes issue #664 which was introduced in #653.
Testing
Improved test for the benchmarks.