Fixed 'passMark' Bug due to incorrect and inconsistent handling #40
Conversation
Modifications: - Removed the static CurrentPassMark and the associated SetCurrentPassMark method in the Arena class, as they were not necessary (redundant) - The passMark property in the Arena class remains and is used directly, avoiding duplication and ensuring that the pass mark value is set correctly from the YAML configuration.
Modifications: - Removed redundant logic related to passMark setting and retrieval. - Updated the ApplyNewArenaConfiguration method to directly use the passMark value from the ArenaConfiguration instance without any additional methods (reduced LoC) - added debug statements strategically to confirm the correct application of the passMark.
Modifications are: - Updated to use the passMark directly from the ArenaConfiguration without relying on any static properties or methods (this was likely causing half the the bug logic) - Added debug logging in OnEpisodeBegin and other relevant methods to ensure the correct passMark is being used and applied throughout the training process. - Removed EpisodeDebugLogs() method as redundancy.
|
nit: In the PR description this is highlighted as a bug fix, but the title of the PR calls it a refactor. I would expect a refactor not to change any functionality - the title should make it clear that this change is fixing a bug and how. Additionally the comment explaining the PR should be explicit about what the behaviour of the bug was, what was causing it and how the change fixes it. It's good to do some tidying up in the course of fixing a bug, and to describe what those changes are, but if you've set out to fix a bug and succeeded that's the main thing! |
alhasacademy96
changed the title
alhasacademy96
changed the title
|
nit: It's good to have detailed descriptions in the commit messages, but commits also need titles otherwise people are faced with this view when they look at the history
|
alhasacademy96
deleted the
fix-passMark-value-discrepancy-per-configuration
branch
|
Yeah so in order to see this bug simply put (as an example) two or three arena configs - set their passMark values individually and check the debug logs on unity. You will see the inconsistencies. I'm surprised how this bug was left unnoticed until now. There will be more comprehensive tests added |
PR Description
The bug was apparent when multiple arenas were defined in the yaml configuration but some of them had incorrect/inconsistent passMark values assigned to them. For example, there are 3 arenas defined, with each 20, 50, 100 passMark values assigned to them. Upon play, the first arena had passMark value of 0, and second arena had a passMark of 20, the third 50.
Proposed change(s)
This PR refactors the handling of the
passMarkproperty across several classes to remove redundancy and simplify the codebase. Specifically, I removed unnecessary static properties and methods related to passMark and ensured that the value is correctly parsed from the YAML configuration and applied directly in the ArenaConfiguration class.In the code, a redundant variable and getter method related to passMark parameter was removed to simply and improve code.
Changes:
Removed Redundant Static Logic:
Direct Property Access:
Updated Class Implementations:
Useful links (Github issues, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments
Key tests carried out:
This PR also significantly simplifies the handling of the passMark property, reducing the potential for bugs and making the code easier to maintain.
Screenshots (if any)