Updated Package#26
Conversation
zsunberg
left a comment
zsunberg
left a comment
There was a problem hiding this comment.
Hi @Aero-Spec , thanks so much for your contributions. Please see my comments for changes that need to be made before merging. Thanks!
| s + a + w.stdev * randn(rng, Vec2) | ||
| end | ||
| end | ||
|
|
||
| function POMDPs.transition(w::CWorld, s::AbstractVector, a::AbstractVector, rng::AbstractRNG) | ||
| ImplicitDistribution(w, s, a) do w, s, a, rng | ||
| s + a + w.stdev * randn(rng, Vec2) |
There was a problem hiding this comment.
| s + a + w.stdev * randn(rng, Vec2) | |
| end | |
| end | |
| function POMDPs.transition(w::CWorld, s::AbstractVector, a::AbstractVector, rng::AbstractRNG) | |
| ImplicitDistribution(w, s, a) do w, s, a, rng | |
| s + a + w.stdev * randn(rng, Vec2) |
transition should not accept rng as an argument; it should return a distribution, and this distribution may have a rand(rng, d) method. This method should be removed.
| function Vec2Distribution(xlim::Tuple{Float64,Float64}, ylim::Tuple{Float64,Float64}) | ||
| xlim::Tuple{Float64, Float64} | ||
| ylim::Tuple{Float64, Float64} | ||
| d::Product |
There was a problem hiding this comment.
The comment should not be removed as it may be helpful in understanding why this is included in the type.
| y = v.ylim[1] + (v.ylim[2]-v.ylim[1])*rand(rng) | ||
| return Vec2(x,y) | ||
| function Base.rand(rng::AbstractRNG, v::Vec2Distribution) | ||
| x = v.xlim[1] + (v.xlim[2] - v.xlim[1]) * rand(rng) |
There was a problem hiding this comment.
The comment should not be removed s it may be helpful in understanding why this function was implemented this way.
| for a in actions(w) | ||
| Qsum = 0.0 | ||
| for j in 1:sol.m | ||
| sp, r = @gen(:sp, :r)(w, s, a, sol.rng) | ||
| Qsum += r + discount(w)*evaluate(val, sp) | ||
| sp = rand(transition(w, s, a, sol.rng)) | ||
| r = reward(w, s, a, sp) | ||
| Qsum += r + discount(w) * evaluate(val, sp) | ||
| end | ||
| best_Qsum = max(best_Qsum, Qsum) | ||
| end | ||
| newdata[i] = best_Qsum/sol.m | ||
| newdata[i] = best_Qsum / sol.m |
There was a problem hiding this comment.
These changes make the solver less general. Is there a reason why you would like to commit these changes? I recommend reverting them.
| s::Union{Vec2, Nothing}=nothing, | ||
| f::Union{Function, Nothing}=nothing, | ||
| g::Union{AbstractGrid, Nothing}=nothing, | ||
| title::Union{String, Nothing}=nothing) |
There was a problem hiding this comment.
Type annotations for function arguments should usually be left off unless they are needed for dispatch. In the new code, s=[1,2] will cause an error, whereas previously it would automatically convert it.
| sp = rand(transition(w, s, a, sol.rng)) | ||
| r = reward(w, s, a, sp) |
There was a problem hiding this comment.
These changes make the solver less general and should be removed.
| s = Vec2(3.0, 2.0) | ||
| a = Vec2(4.0, 2.0) | ||
| rng = MersenneTwister(19) | ||
| d = transition(w, s, a, rng) |
There was a problem hiding this comment.
rng should be an argument to rand rather than transition
|
Hi @zsunberg , thanks for the feedback. I just made the requested changes. Please let me know if everything looks good now or if there is anything else you would like me to adjust. Thanks! |
zsunberg
left a comment
zsunberg
left a comment
There was a problem hiding this comment.
Thanks @Aero-Spec !
A general note about pull requests: Many of your changes in this PR are style changes (e.g. adding spaces, etc.) Most of these changes seem like good changes. However, when submitting PRs, it is usually best to separate style changes and functionality changes into different PRs. If there are functionality changes, the style changes can make it hard to see the important functionality changes and verify that they are doing the right thing. After the functionality PR is accepted, you can then submit a style PR if desired.
Anyways, thanks for the contribution!!
|
Thanks for the tip @zsunberg ! I appreciate your advice. I’ll be sure to separate style changes and functionality changes into different PRs moving forward to keep things clear and review-friendly. I appreciate you taking the time to guide me. Looking forward to contributing more. Thanks again! |
2 participants
Uh oh!
There was an error while loading. Please reload this page.