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

Make FluxApproximator work with QBasedPolicy #1075

Merged
merged 21 commits into from
May 13, 2024
Merged

Conversation

jeremiahpslewis
Copy link
Member

@jeremiahpslewis jeremiahpslewis commented May 8, 2024

  • Drop legal_action_space_mask methods for MinimalActionSet case
  • Clean up state syntax so FluxApproximator works with QBasedPolicy
  • Use full state(env, Observation, DefaultPlayer) calls in RLEnvs

PR Checklist

  • Update NEWS.md?
  • Unit tests for all structs / functions?
  • Integration and correctness tests using a simple env?
  • PR Review?
  • Add or update documentation?
  • Write docstrings for new methods?

Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.57%. Comparing base (184d441) to head (86fb33d).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1075       +/-   ##
===========================================
+ Coverage   31.26%   81.57%   +50.31%     
===========================================
  Files          72        3       -69     
  Lines        2783       38     -2745     
===========================================
- Hits          870       31      -839     
+ Misses       1913        7     -1906     

see 73 files with indirect coverage changes

@jeremiahpslewis jeremiahpslewis changed the title Fix devcontainer Add missing legal_action_space_mask default methods May 8, 2024
@jeremiahpslewis jeremiahpslewis changed the title Add missing legal_action_space_mask default methods Add missing legal_action_space_mask default methods May 8, 2024
@jeremiahpslewis jeremiahpslewis enabled auto-merge (squash) May 8, 2024 14:45
@jeremiahpslewis jeremiahpslewis changed the title Add missing legal_action_space_mask default methods Droplegal_action_space_mask methods for MinimalActionSet case May 8, 2024
@jeremiahpslewis jeremiahpslewis changed the title Droplegal_action_space_mask methods for MinimalActionSet case Drop legal_action_space_mask methods for MinimalActionSet case and clean up state syntax May 8, 2024
@jeremiahpslewis jeremiahpslewis changed the title Drop legal_action_space_mask methods for MinimalActionSet case and clean up state syntax Drop legal_action_space_mask methods for MinimalActionSet case and clean up state syntax so FluxApproximator works with QBasedPolicy May 9, 2024
@jeremiahpslewis jeremiahpslewis requested a review from HenriDeh May 9, 2024 07:34
@jeremiahpslewis
Copy link
Member Author

@HenriDeh Would love your review on this when you have a chance. :)

@jeremiahpslewis jeremiahpslewis changed the title Drop legal_action_space_mask methods for MinimalActionSet case and clean up state syntax so FluxApproximator works with QBasedPolicy Make FluxApproximator work with QBasedPolicy May 10, 2024
Copy link
Member

@HenriDeh HenriDeh left a comment

Choose a reason for hiding this comment

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

See above

@jeremiahpslewis
Copy link
Member Author

Thanks! Added an explanation.

@jeremiahpslewis jeremiahpslewis requested a review from HenriDeh May 13, 2024 09:28
@jeremiahpslewis jeremiahpslewis merged commit 89a46d9 into main May 13, 2024
40 checks passed
@HenriDeh
Copy link
Member

Thanks! Added an explanation.

I think you did not push it ?

@jeremiahpslewis
Copy link
Member Author

Thanks! Added an explanation.

I think you did not push it ?

Sorry, was unclear. I meant a comment to your review question

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.

2 participants