-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: adding info - is_success #46
Conversation
Got this to work using your Python changes and with some small changes on the plugin side to make this optional. Instead of changing your PR (you might be working on other changes), I will post a PR to your fork when done, so you can check and merge it. This will also warrant a small tutorial, I will add it. As expected this can be a very useful feature (in some cases surpassing the reward as the most important metric) even though it's a relatively quick change code-wise. Thanks for working on the issue! |
You can add to the PR if you want, since this isn’t urgent and I can also push it sometime next week. |
OK, I'll add to the PR directly (easier), and will write when it's done after. The feature is definitely not urgent, and there are some tests I'd like to do - especially for the tutorial part (although that will be a new PR). It seems we might not need to modify the training script - even though the docs suggest adding success to the monitor is needed. It's possible it may be needed in some versions, so I'll make sure to point to that in the tutorial. |
Makes info optional, changes some comments to regions, and applies gdformat
Makes the info optional and adds regions
Partially returns manual formatting, the enum comments were outside after automatic format.
Removes the reward on reset, as it shouldn't be necessary. Applies some auto formatting (except the enum).
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.
Changes should be done, needs a bit more testing.
I mostly made the info optional, removed the reward on reset (as it shouldn't be necessary), and tested/added the tutorial in the other PR (main repository).
Pending review from @edbeeching before merging (feel free to merge after). If I have some more time to test in the meantime and find any issues (or if anyone else does), feel free to post here.
Great work @GianiStatie
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
Adding the ability to pass game info to the Python code, specifically if a run
is_success
. This gives more info to the developer when a simulation ends and they want to know the outcome.This MR links to:
And blocks: