-
Notifications
You must be signed in to change notification settings - Fork 125
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
Implement EventQueueABM #917
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #917 +/- ##
==========================================
- Coverage 92.26% 90.51% -1.75%
==========================================
Files 33 36 +3
Lines 2274 2331 +57
==========================================
+ Hits 2098 2110 +12
- Misses 176 221 +45 ☔ View full report in Codecov by Sentry. |
I am confused about the existence of I would argue that if a user wants "agent steps" and "model steps" they should just go ahead and use the StandardABM and hence here we should focus on just the quing system and simplify everything., |
I agree 👍 |
event_function! = abmevents(model)[agent_type][event_idx] | ||
event_function!(agent, model) | ||
!haskey(agent_container(model), id) && return | ||
propensities = abmrates(agent, model) .* nagents(model) |
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.
It's not really clear to me why here we multiply by nagents(model)
, shouldn't it be the number of agents of the related agent type?
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.
actually to me is not clear why applying a function such as nagents
in general? shouldn't the user define his own rates? Given that we let choosing a function then every transformation of the propensities seems not useful. But it's not entirely clear to me how the algorithm should work, so I could be mistaken
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.
@MA-Ramirez @Datseris I'd need some help on this if you can give some it would really appreciated :-)
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.
If nobody has insights on this I will remove it, it is in any case possible to achieve through passing a function in abmrates
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.
Sorry for the late reply. Yes, you should remove this part and if the user needs to they can call nagents
in abmrates
.
@@ -8,10 +8,11 @@ In this page we list the remaining API functions, which constitute the bulk of A | |||
Besides the generic interface of [`AgentBasedModel`](@ref), and the [`StandardABM`](@ref), highlighted in the tutorial, there is also: | |||
|
|||
```@docs | |||
UnremovableABM | |||
EventQueueABM |
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 think we should move AgentBasedModel
here as well, and only expand the docstring of StandardABM
in the tutorial. In the tutorial we just mention explicitly part of the interface by showing how it works in terms of examples. E.g., do something like model[2]
. I can do this after this PR is merged.
@@ -138,7 +138,7 @@ end | |||
# The first type parameter of any `ABM` subtype must be the space type. | |||
spacetype(::ABM{S}) where {S} = S | |||
|
|||
agenttype(model::ABM) = notimplemented(model) | |||
tuple_agenttype(model::ABM) = getfield(model, :agents_types) |
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.
Why are we renaming agenttype
? I think tuple_agenttype
is a bit of a mouthful as a getter. I think we can just accept that agenttype
can return either a type or a union, and if not we can have agenttypes
for unions and agenttype
using only
to ensure a single return type.
@Tortar what is remaining for this to be finished? It's the last PR before v6, right? |
Mainly some docstrings need to be finished and we need to add some more tests for the new model type. We agreed with @fbanning to put also #934 in the v6 milestone. I'm a bit overwhelmed by other tasks at the moment, I think I can work on this monday. But if someone else wants to help feel free to add your commits to the branch |
Can I ask what happened with the ConcurrentSim implementation? Did anyone try doing a comparison? I looked into CS.jl and it was rather complicated. I think it raises the complexity barrier for contributers even if none of the internals are exposed. |
I think that we can add the CS.jl integration later, I'm not sure that internal with CS.jl will become too complicated, maybe it is so but it is worth trying in my opinion. I don't think it's a priority though |
At one point we can make a performance comparison with "from scratch continuous abm", "Agents-v6 continuous abm", and a "CS Agents continuous abm" and see if it's worth it, and if so how much. |
I can take over this PR. I realized there are some things we need to improve. For example, if an agent is added to the model via e.g., I also want to do two more improvements: using the concept of probability mass to decide how events are chosen, and simplifying significantly the events. I will add a generic type |
I have had great progress in the The new code gives more power to the user (more possible configurations and flexibility) while requiring the user to input less information due to two default options! |
Closing in favor of #940 |
Fixes #902
Still very bad but the new example at least works :-)