Conversation
add convert_s function
Codecov ReportBase: 68.23% // Head: 67.62% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
==========================================
- Coverage 68.23% 67.62% -0.61%
==========================================
Files 10 10
Lines 447 451 +4
==========================================
Hits 305 305
- Misses 142 146 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
zsunberg
left a comment
zsunberg
left a comment
There was a problem hiding this comment.
Awesome, thanks for the contribution! As in the rocksample PR, can you add a test?
Please also see my inline comment.
Finally, as in rocksample, it would be great to add the convert_s for the opposite direction.
src/LaserTag.jl
Outdated
|
|
||
| # add a function to transform a LTState to a vector | ||
| function POMDPs.convert_s(T::Type{<:AbstractArray}, s::LTState, p::LaserTagPOMDP) | ||
| return convert(T, vcat(s.robot, s.opponent, [s.terminal])) |
There was a problem hiding this comment.
| return convert(T, vcat(s.robot, s.opponent, [s.terminal])) | |
| return convert(T, vcat(s.robot, s.opponent, s.terminal)) |
I don't think you need to create a vector with s.terminal in it. That will cause an additional allocation.
(additionally, I think vcat itself might cause an allocation, which is disappointing. You might be able to eliminate all allocations with StaticArrays, but that might take a bit of learning. There are many tools to track allocations including BenchmarkTools.@btime
There was a problem hiding this comment.
Thanks for the info!
zsunberg
left a comment
zsunberg
left a comment
There was a problem hiding this comment.
@yangyou95 if you would like to finish this PR - that would be helpful! I added one suggestion to avoid allocating an array, but the most important change still needed is to add tests! Let me know if you need pointers on how to do this!
src/LaserTag.jl
Outdated
|
|
||
| # transform a vector to a LTState | ||
| function POMDPs.convert_s(T::Type{LTState}, v::AbstractArray{Float64}, p::LaserTagPOMDP) | ||
| return LTState([v[1], v[2]], [v[3], v[4]], v[5]) |
There was a problem hiding this comment.
| return LTState([v[1], v[2]], [v[3], v[4]], v[5]) | |
| return LTState(Coord(v[1], v[2]), Coord(v[3], v[4]), v[5]) |
There was a problem hiding this comment.
Hello @zsunberg! Sorry, I totally forgot this change after Xmas... I updated and pushed it! Hope what did is correct.
Pls let me know if everything is ok.
|
Hi @zsunberg, I’ve made a small update to the convert_s function to use SVector for better performance, and I also added corresponding tests in the runtests file. Also, sorry for the long delay (about 2 years!) — I was caught up preparing for my PhD defense at the time. |
zsunberg
left a comment
zsunberg
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I hope that you passed your defense!
I just removed some comments that didn't add any additional clarification, and made the tests a bit more specific. Otherwise it looks good. I will merge once the tests pass.
2 participants
It seems like there is no convert_s() function for the LaserTag problem.
Adding a convert_s() function, transforming a LTState to a vector.