Skip to content
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

Refactor reward logging to use running totals #124

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nathandsouza10
Copy link

  • Removed list-based reward accumulation
  • Implemented running total/count for individual and group rewards
  • Streamlined TensorBoard logging to use calculated averages

- Removed list-based reward accumulation
- Implemented running total/count for individual and group rewards
- Streamlined TensorBoard logging to use calculated averages
@jaredvann
Copy link
Contributor

@nathandsouza10 Hi, thank you for taking the time to create this PR. We are not completely clear on what the overall benefits of these changes are. Is the goal optimisation of memory usage from not needing to store lists of reward values?

@nathandsouza10
Copy link
Author

@nathandsouza10 Hi, thank you for taking the time to create this PR. We are not completely clear on what the overall benefits of these changes are. Is the goal optimisation of memory usage from not needing to store lists of reward values?

Hi, thank you for considering my PR. Yes, the primary goal of these changes, albeit small, is to optimize memory usage. By moving away from storing extensive lists of reward values and implementing a running total and count for each agent, we significantly reduce the memory overhead. This is particularly beneficial in scenarios with many agents or long-running environments.

@jaredvann
Copy link
Contributor

jaredvann commented Dec 11, 2023

Thanks! Please see one small comment below.

To be able to contribute to this repo and merge this PR you will need to first sign the JPMorgan Chase Contributor License Agreement (CLA). This can be found at https://github.com/jpmorganchase/.github/blob/main/jpmc-cla-20230406.md. Instructions can be found at the bottom of the file.

Please let me know if you have any questions.

@nathandsouza10
Copy link
Author

any questions.

Thank you for guiding me through the contribution process. I wanted to inform you that I have already completed the submission of the JPMorgan Chase Contributor License Agreement (CLA). This was done on Friday, December 5th, and I sent it to [email protected]. Please do let me know if there is anything else I need to do or if there are any further steps to finalize my contribution. I’m looking forward to actively participating in the project!

@leoardon
Copy link
Contributor

@nathandsouza10 Thanks for raising the request to contribute, it might need some time to be reviewed and accepted. We will follow up on this.
Could you please make the change mentioned and make sure it works as expected?

Following review feedback, the explicit initialisation of logged_rewards[agent_id] has been removed.
@leoardon
Copy link
Contributor

@nathandsouza10 Good news, I see your CLA request has gone through and you're now an official contributor! 🎉 ✨

Could you also add unit tests to make sure your changes are working please? I see for example that the logged_rewards are still initialized as a list in the __init__ method l.89.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants