Allow using default values in agents macro #833
Conversation
Codecov Report
@@ Coverage Diff @@
## main #833 +/- ##
=======================================
Coverage 70.13% 70.13%
=======================================
Files 42 42
Lines 2719 2719
=======================================
Hits 1907 1907
Misses 812 812
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@fbanning I'd like to ask you what do you think about the pr since I saw you opened the linked issue :-) |
|
To me this looks as clean and straightforward as possible. |
|
Did you run any performance checks on the |
|
yes, tried to see if there were any regression with micro-benchmarks on agent creation and there wasn't any. Thanks for the review anyway, I merge then! |
|
wow what a big improvement! 🎉 |
|
does this impact in any way Actually, I don't know. How does this work...? |
| @@ -35,6 +35,13 @@ using Test, Agents, Random | |||
| end | |||
| @test Fisher <: AbstractHuman | |||
| @test :fish_per_day ∈ fieldnames(Fisher) | |||
|
|
|||
There was a problem hiding this comment.
Here definitely more tests should have been added to see how this works with add_agent! that automatically creates agents.
There was a problem hiding this comment.
yes, it is actually something I didn't consider when doing the pr :D, it seems that it shouldn't have any effect on the version with args, I'm also pretty sure that currently if we pass anything in kwargs it will throw error (even before this pr)
There was a problem hiding this comment.
But we can support both ways with:
function add_agent!(
pos::ValidPos,
A::Type{<:AbstractAgent},
model::ABM,
properties::Vararg{Any, N};
kwproperties...,
) where {N}
id = nextid(model)
if isempty(kwproperties)
newagent = A(id, pos, properties...)
else
newagent = A(; id = id, pos = pos, kwproperties...)
end
add_agent_pos!(newagent, model)
endIt should be as fast as before since the branch prediction is pretty easy so the compiler should be able to infer what to do, but we should probably verify it
There was a problem hiding this comment.
indeed I don't see any meaningful difference in our model comparisons
# before this modification
Agents.jl WolfSheep-small (ms): 2.8282849999999997
Agents.jl WolfSheep-large (ms): 99.588342
Agents.jl Flocking-small (ms): 9.377578999999999
Agents.jl Flocking-large (ms): 67.284021
Agents.jl Schelling-small (ms): 0.453293
Agents.jl Schelling-large (ms): 4.035073
Agents.jl ForestFire-small (ms): 0.767832
Agents.jl ForestFire-large (ms): 16.152233
# after this modification
Agents.jl WolfSheep-small (ms): 2.82549
Agents.jl WolfSheep-large (ms): 99.25996699999999
Agents.jl Flocking-small (ms): 8.799429
Agents.jl Flocking-large (ms): 67.878292
Agents.jl Schelling-small (ms): 0.455848
Agents.jl Schelling-large (ms): 4.059518
Agents.jl ForestFire-small (ms): 0.776428
Agents.jl ForestFire-large (ms): 16.248959
This tries to split in two parts #831 since the part with the consts variable is more hacky, but using default values is very easy and we can support it almost with no changes