Simulate #42
Simulate #42
Conversation
|
|
||
| function _run_callbacks(callbacks::C, rec::EntityQueueRecord{S,D,I}, roadway::R, models::Dict{I,M}, tick::Int) where {S,D,I,R,M<:DriverModel,C<:Tuple{Vararg{Any}}} |
There was a problem hiding this comment.
Maybe we still need the QueueRecord version while rec is not deprecated?
|
|
||
| veh_state_p = propagate(veh, a, roadway, timestep) | ||
|
|
||
| push!(scenes[tick + 1], Entity(veh_state_p, veh.def, veh.id)) |
There was a problem hiding this comment.
can we use fill! and indexing [i] instead of empty! and push!? May be more efficient and kind of feels cleaner to me
There was a problem hiding this comment.
Actually the push! function on the Frame objects is not allocating: https://github.com/sisl/Records.jl/blob/master/src/frames.jl#L56
I also thought fill! would be cleaner but it creates reference to the same scene object.
src/simulation/simulation.jl
Outdated
| end | ||
|
|
||
| if !(callbacks === nothing) && _run_callbacks(callbacks, scenes, roadway, models, tick) | ||
| final_tick = tick + 1 |
There was a problem hiding this comment.
we could probably get rid of the final_tick variable alltogether by using scenes = scenes[1:tick+1] here (or some other function for deleting remaining entries, ideally something that does not allocate new memory for scenes) and then simply returning return scenes at the end.
There was a problem hiding this comment.
I am not a fan of deleting entries, since scenes can be given in arguments, would return scenes[1:tick+1] in the if statement work?
There was a problem hiding this comment.
yes, even better I'd say
test/test_simulation.jl
Outdated
|
|
||
| simulate!(scene, roadway, models, n_steps, dt) | ||
|
|
||
| simulate!(scene, roadway, models, n_steps, dt, callbacks=(CollisionCallback(),)) |
There was a problem hiding this comment.
do we want to call both versions of simulate here? what exactly is being tested? Maybe at least test type stability with @inferred
There was a problem hiding this comment.
I updated the tests, now it is checking that the callback is actually called and that the list of scenes is smaller than expected.
There was a problem hiding this comment.
This is an awesome idea! In fact, I was not using simulate! in my work and instead just running a loop that calls get_actions! and tick! and then pushes the updated scene into a list of scenes because the Queuerecord made my head hurt. So, this is brilliant. I am not a software engineering wizard like y'all so I don't have specific feedback on code speed or coding standards.
Also, it is great that now we have using Random within AutomotiveModels.jl.
|
@raunakbh92 if you are not using |
|
I am using the |
Change the
simulate!function to return a vector of scenes. The function is faster, type stable, and the vector of scenes is easier to manipulate than theQueueRecordobject.