-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EventQueue ABM v3 #940
EventQueue ABM v3 #940
Conversation
1 type RockPaperScissors julia> @benchmark step!($model) seconds=1 evals=1 samples=10^6
BenchmarkTools.Trial: 471880 samples with 1 evaluation.
Range (min … max): 271.000 ns … 2.536 ms ┊ GC (min … max): 0.00% … 98.72%
Time (median): 551.000 ns ┊ GC (median): 0.00%
Time (mean ± σ): 597.350 ns ± 3.697 μs ┊ GC (mean ± σ): 0.89% ± 0.14%
▁█▁▂ ▄█▁▆▃ ▁▁
▁▂▃████▆▃▅▆▅█████▅██▅██▅█▇▄▆▆▃▅▅▃▄▄▂▃▃▂▃▃▂▂▂▂▂▂▁▂▂▁▂▁▁▁▁▁▁▁ ▃
271 ns Histogram: frequency by time 1.25 μs <
Memory estimate: 224 bytes, allocs estimate: 6. 3 types Rock, Paper, Scissors julia> @benchmark step!($model) seconds=1 evals=1 samples=10^6
BenchmarkTools.Trial: 397554 samples with 1 evaluation.
Range (min … max): 160.000 ns … 2.875 ms ┊ GC (min … max): 0.00% … 98.36%
Time (median): 852.000 ns ┊ GC (median): 0.00%
Time (mean ± σ): 925.216 ns ± 9.526 μs ┊ GC (mean ± σ): 3.92% ± 0.38%
▁▆▃▃▄█▃▃▃█▃▃▂▅ ▁
▂▂▂▂▂▂▁▂▂▂▂▂▂▂▂▃▃▃▄▇██████████████████▇█▆▆▅▆▄▄▄▄▃▃▃▃▃▂▂▂▂▂▂ ▄
160 ns Histogram: frequency by time 1.49 μs <
Memory estimate: 0 bytes, allocs estimate: 0. it helps, but not that much as expected |
It's almost a 2x performance increase, isn't this huge? I would say so. |
In general this is bad for memory usage and doesn't allow multiple dispatch on the agent type, so I think this should be a optional feature: I discovered some time ago this package: https://github.com/MasonProtter/SumTypes.jl, which could be the solution. This doesn't require specifying default values which is really cool. But still I don't totally understand how the package can be adapted for Agents.jl :D (actually almost yes, but it needs some work, looking at the code of SumTypes in https://github.com/MasonProtter/SumTypes.jl#performance seems pretty much it actually) |
in my gergo it is a cut of 40% of the time :D |
Actually I also understood now that it's harder to replicate the current behaviour with this model type with only one-for-all type: how do you implement this? We would need to change the source code for this movement_event = AgentEvent(
action! = move!, propensity = movement_propensity,
types = Union{Scissors, Paper}, timing = movement_time
) removing the # 3 types
julia> @time step!(model, 10000.0)
0.258423 seconds (2.78 M allocations: 166.209 MiB, 12.64% gc time)
# 1 type
julia> @time step!(model, 10000.0)
0.182622 seconds (2.42 M allocations: 100.065 MiB, 12.93% gc time) I think that implementing a kind of different behaviour for a one-for-all type is something should be done in a different PR though, because I think we should first find a good implementation, SumTypes.jl should help |
Sure. Let's finish this and merge it in. After all, my plan is to add a clear information in the docs that the event queue is experimental and may change in the future without triggering a new major version. We need users to test it I feel. |
Ok, |
I don't have the time these days to finish it, if someone has please go on! |
ok, it seems to me that it is hard to make everything work all at once, so I marked this new model type as experimental both in the docstring and when creating an instance of it, I also took care to list some functionality limitations of the current implementation. I think that we should open an issue listing which functions are still not usable with an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR, even if you have put in more work in it... So you can approve and merge instead.
Yes I agree, we can merge and finalize before releasing v6, after the visualizations PR is finished. Sorry, I am overwhelemd with both work and personal matters so i've been away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR, even if you have put in more work in it... So you can approve and merge instead.
Yes I agree, we can merge and finalize before releasing v6, after the visualizations PR is finished. Sorry, I am overwhelemd with both work and personal matters so i've been away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't worry, loved to work on it (even if it was harder than expected :Q)
I will open an issue listing functions we still need to implement for this model type soon |
Closes #760
Closes #902
Alright, this is pretty cool. To review this PR read the following three things:
StandardABM
inmodel_standard.jl
EventQueueABM
inmovel_event_queue.jl
event_rock_paper_scissors.jl
file for an example application.So this PR superseeds #917 by changing a bit the model declaration.
We now have events, that include an action (
agent_step!
), can be applied to arbitrary agents. Event generation can happen in two ways:add_event!
orgenerate_event_to_queue!
.This is the best of all worlds. It solves all problems. It allows the model to work without the user having to provide a list of initial events. But also allows them to take full control of event scheduling if they want.
Also, it allows going beyond the standard Gillespie simulations by allowing the event times to be arbitrary functions. No longer limited to an exponentially distributed time.
By default however we do have an exponentially distributed time.
Please review by reading the three files I mention at the start! The PR is not finished of course, there is more docmentation to be added and tests. However, I would argue, if we agree with the interface we should merge and we should ask for tests in another PR by opening an issue.
cc @MA-Ramirez