WIP: add nfsp algorithm and relative experiment
#375
Conversation
peterchen96
force-pushed
the
peter/kuhn_nfsp
branch
from
b3befda to
5503d31
Compare
| savefig("assets/JuliaRL_NFSP_KuhnPoker.png")#hide | ||
|
|
||
| #  |
There was a problem hiding this comment.
These are used to generate the progress plot. Whether it is needed depends on how you write the cover field in the front matter (see my previous comment).
|
|
||
| See the paper https://arxiv.org/abs/1603.01121 for more details. | ||
| """ | ||
| export NFSPAgent, NFSPAgents |
There was a problem hiding this comment.
I don't think users need to directly use NFSPAgent, so no need to export this one.
| mutable struct NFSPAgents <: AbstractPolicy | ||
| agents::Dict{Any, AbstractPolicy} | ||
| end |
There was a problem hiding this comment.
We don't use plural case NFSPAgents to differentiate with NFSPAgent.
Since we don't really need to export NFSPAgent, we can maybe rename NFSPAgent to NFSPAgentInstance?
| """ | ||
| Neural Fictitious Self-Play (NFSP) agent implemented in Julia. | ||
|
|
||
| See the paper https://arxiv.org/abs/1603.01121 for more details. | ||
| """ |
There was a problem hiding this comment.
I believe this docstring should go to NFSPAgents. Please follow the Julia guideline https://docs.julialang.org/en/v1/manual/documentation/
| mutable struct NFSPAgent <: AbstractPolicy | ||
| η | ||
| rng | ||
| rl_agent::Agent | ||
| sl_agent::Agent | ||
| end |
There was a problem hiding this comment.
Is this a usable policy? Otherwise there's no need to <: AbstractPolicy.
| using Distributions: TruncatedNormal | ||
|
|
||
| mutable struct NFSPAgents <: AbstractPolicy | ||
| agents::Dict{Any, AbstractPolicy} |
There was a problem hiding this comment.
Just a question: does it really accept any type of AbstractPolicy?
| sl_agent::Agent | ||
| end | ||
|
|
||
| function initW(out_size, in_size) |
There was a problem hiding this comment.
initW is so common a name that might cause conflict with other algorithms.
| function initW(out_size, in_size) | |
| function _NFSP_initW(out_size, in_size) |
There was a problem hiding this comment.
Thanks for your reviews and sorry for the late response.
I don't think users need to directly use
NFSPAgent, so no need to export this one.We don't use plural case
NFSPAgentsto differentiate withNFSPAgent.Since we don't really need to export
NFSPAgent, we can maybe renameNFSPAgenttoNFSPAgentInstance?
Here, NFSPAgent is the core policy to play the game. In my view, agents in one multi-agent experiment may choose different policies to play the game. (maybe just like players can use different schemes in one game)
And NFSPAgents is more like a special MultiAgentManager in which all agents choose nfsp policy to learn the best response. For more clarity, I'll extract 'NFSPAgents' and its relative methods to a new file and renamed it as 'NFSPAgentManager'.
You can follow this format here:
I believe this docstring should go to
NFSPAgents. Please follow the Julia guideline https://docs.julialang.org/en/v1/manual/documentation/
Thanks for your reminder. I'll supplement the description about NFSPAgent and its experiment later.
initWis so common a name that might cause conflict with other algorithms.
Thank you for pointing it out. Maybe I'll rename it as _TruncatedNormal because of the used distribution.
| # author: "[Peter Chen](https://github.com/peterchen96)" | ||
| # --- | ||
|
|
||
| #+ tangle=false |
| using Flux | ||
| using Flux.Losses | ||
|
|
||
| mutable struct ResultNEpisode <: AbstractHook |
There was a problem hiding this comment.
Why is it set to a subtype of AbstractHook here?
| episode::Vector{Int} | ||
| results | ||
| end | ||
| recorder = ResultNEpisode([], []) |
There was a problem hiding this comment.
Please note that it will be set to the global variable in the ReinforcementLearningExperiments.jl package once tangle=true. And obviously, this is not what you want. Better to move it into the internal of the experiment.
| env = KuhnPokerEnv() | ||
| states = [ | ||
| (), (:J,), (:Q,), (:K,), | ||
| (:J, :Q), (:J, :K), (:Q, :J), (:Q, :K), (:K, :J), (:K, :Q), | ||
| (:J, :bet), (:J, :pass), (:Q, :bet), (:Q, :pass), (:K, :bet), (:K, :pass), | ||
| (:J, :pass, :bet), (:J, :bet, :bet), (:J, :bet, :pass), (:J, :pass, :pass), | ||
| (:Q, :pass, :bet), (:Q, :bet, :bet), (:Q, :bet, :pass), (:Q, :pass, :pass), | ||
| (:K, :pass, :bet), (:K, :bet, :bet), (:K, :bet, :pass), (:K, :pass, :pass), | ||
| (:J, :pass, :bet, :pass), (:J, :pass, :bet, :bet), (:Q, :pass, :bet, :pass), | ||
| (:Q, :pass, :bet, :bet), (:K, :pass, :bet, :pass), (:K, :pass, :bet, :bet), | ||
| ] # collect all states |
There was a problem hiding this comment.
How about adding another state representation to this environment?
| ) | ||
| @assert NumAgentStyle(env) isa MultiAgent | ||
| @assert DynamicStyle(env) === SEQUENTIAL | ||
| @assert RewardStyle(env) === TERMINAL_REWARD |
|
Sorry for late response. Since this PR’s implementation was totally incorrect, I would close this and create a new PR to record my implementation and relative experiment. |
add
nfspalgorithm and test it on julia versionKuhnPokerEnv, however, the result is not well for now. #342