Skip to content

Add SARS tdlearning back to lib #1050

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

Merged
merged 21 commits into from
Mar 20, 2024
Merged

Add SARS tdlearning back to lib #1050

merged 21 commits into from
Mar 20, 2024

Conversation

jeremiahpslewis
Copy link
Member

@jeremiahpslewis jeremiahpslewis commented Mar 15, 2024

  • One of a couple pull requests on the way to getting plain vanilla q-learning working
  • Drops optimiser from TabularApproximator, too many unforced errors with it specified
  • Approximator becomes FluxModelApproximator for clarity
  • TabularApproximator and FluxModelApproximator are independent subtypes of AbstractLearner

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 Mar 15, 2024

Codecov Report

Attention: Patch coverage is 80.35714% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 57.50%. Comparing base (55f60b0) to head (a0330b8).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
+ Coverage   55.68%   57.50%   +1.82%     
==========================================
  Files          69       71       +2     
  Lines        2726     2751      +25     
==========================================
+ Hits         1518     1582      +64     
+ Misses       1208     1169      -39     
Files Coverage Δ
...ementLearningCore/src/policies/agent/agent_base.jl 71.42% <ø> (ø)
...e/src/policies/learners/flux_model_approximator.jl 100.00% <100.00%> (ø)
...Core/src/policies/learners/tabular_approximator.jl 100.00% <100.00%> (ø)
...ementLearningFarm/src/ReinforcementLearningFarm.jl 100.00% <ø> (ø)
...arningCore/src/policies/learners/target_network.jl 95.83% <80.00%> (ø)
...ntLearningCore/src/policies/learners/td_learner.jl 91.30% <91.30%> (ø)
...rcementLearningCore/src/policies/q_based_policy.jl 71.42% <66.66%> (+71.42%) ⬆️
.../src/policies/explorers/epsilon_greedy_explorer.jl 23.45% <14.28%> (+23.45%) ⬆️

... and 3 files with indirect coverage changes

@jeremiahpslewis jeremiahpslewis enabled auto-merge (squash) March 19, 2024 20:03
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.

LGTM. Good idea naming the FluxApproximator. One day this could be moving to a Flux extension.

@jeremiahpslewis jeremiahpslewis merged commit bf37d4d into main Mar 20, 2024
13 checks passed
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