Skip to content

Conversation

@sash-a
Copy link
Collaborator

@sash-a sash-a commented Feb 27, 2025

What?

A nice refactor to reduce the nesting used in PPO and generally make the code more understandable.

Why?

I wanted to test Claude code and am pleasantly surprised with the results.

How?

I asked Claude code to refactor the code and reduce the nesting and it's done well

Note:

This doesn't need to go in now, we should decide how quickly we want to merge this considering all the other PPO based systems.
Any style changes you've wanted should be added to this PR - so check carefully!

@sash-a sash-a requested a review from Copilot February 27, 2025 11:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors parts of the PPO configuration by introducing a new configuration check to reduce nesting and improve code clarity.

  • Added a new configuration check function "check_anakin_ppo_config" to validate PPO configurations.
  • Improved configuration validation with additional assert statements.

Reviewed Changes

File Description
mava/utils/config.py Added a new function for PPO configuration validation with two asserts, one of which contains a typo in the error message.

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@sash-a
Copy link
Collaborator Author

sash-a commented Mar 18, 2025

If we also pull out the _update_step function we could import that into the sebulba systems and use it as the learning function reducing some code duplication, but making the sebulba systems less single file. However I feel those files are getting so long that this might be a good option - it's kinda a question of where do we draw the line on importing logic 🤔

@SimonDuToit
Copy link
Contributor

It seems to have deleted a bunch of the docstrings when it moved the functions around 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants