Create abmtime function for StandardABM
Closed this issue · 17 comments
This is practically costless, and actually somewhat needed: run! and offline_run! restart from 0 in the step column each time they are called. Apart from that, this is a function some people would implement by themselves anyway to track the time of the ABM, and implementing it would make StandardABM more homogeneous with the new EventQueueABM which has one.
This means that this is a breaking change though if we change those two functions to instead use the time of the model
cc @Datseris
yes i've been thinking about this as well, to add a timer in standardABM as well. Now run! and co should start from the model timer which itself starts at 0 but is never reset (unless re-creating the model of course).
and so I guess also step! with a function should change:
Step the model forwards until
f(model, t)returnstrue,
wheretis the current amount of time the model has been evolved
for, starting from 0.
(the docstring is even not very clear in my opinion)
I would remove the t dependency and just do f(model), the user could use abmtime inside if wants to. Notice that this could be non breaking, since we can easily enough detect which one of the two signatures is called, it is just a deprecation
Okay, but it has to be breaking. It is bad for performance to check if a user gave in a function expecting 1 or 2 arguments. Type unstable. (I know from DynamicalSystems.jl)
actually it seems not bad with this trick:
julia> h(x) = x
h (generic function with 1 method)
julia> h(x, y) = x + y
h (generic function with 2 methods)
julia> t(f) = try; f(1); return true; catch; f(1,2); return false; end;
julia> using BenchmarkTools
julia> @btime t($h)
1.182 ns (0 allocations: 0 bytes)
true(same when h(x) is not defined)
this is a missleading benchmark because you are passing constant literals to f: 1, 2. The compiler optimizes everything away and understands that f is called with 1, whose value is true. Everything is inlined away to the trivial t(f) = true.
it is well known (stated in the manual in performance tips) that one should not use try blocks inside performance critical code, doubly more so not inside a for loop which is what step! does. Here the try block is simply compiled away!
good observation, but I guess we are still in a similar situation since I think model is constant propagated, and I would use 0 for t, let's revise this in the future
notice also that we would call it outside the for loop inside step!, at the start of the function, just a warning there is enough
applicable is probably more robust though:
julia> f(x) = x
f (generic function with 1 method)
julia> t(f, x, y) = applicable(f, x, y) ? true : false
t (generic function with 1 method)
julia> using BenchmarkTools
julia> @btime t($f, $1, $1)
93.312 ns (0 allocations: 0 bytes)
false(the try catch trick takes microseconds when the catch block is executed instead...otherwise it is very fast though, so it could become a good reason for the user to update the code :D)
or, we could simply break nothing. Make it so that the API is: f is user input, and f(model, t) returns true is when evolution stops. t is always the total amount of time elapsed since the start of the run! call. So, it does not care about the current model time.
Or, t is the same as the model time. Up to us to choose.
Since the model time is a new concept, either choice is non-breaking, given that we didn't explain this well enough originally. (What is breaking or not is defined by what the docs say)
Or,
tis the same as the model time. Up to us to choose.
In this case:
- it is redundant to pass it since it can be accessed with abmtime(model)
- (!) it actually changes the behaviour to use
tas the model time, even if it is not stated in the docs, so this would be a silent breaking change, even if the docs weren't clear - it is a bit more sensible to me
f(model)since the stopping condition would probably be something unrelated to the time
On the other hand, leaving t as it is now it would be better in a sense since something like this is more easy to implement:
f(m, t) = stopping_condition_unrelated_to_time(m) || t > 1000
so in the end I think that keeping f(model, t) as it is now, and just improving the docs should be enough 👍
it is redundant to pass it since it can be accessed with abmtime(model)
Sure, but it keeps the API the same, so it is non breaking in this case.
- even if it is not stated in the docs, so this would be a silent breaking change, even if the docs weren't clear
I don't think this is a breaking change. This is what I am trying to say: whether something is "breaking" or not is defined by the docs. No by what the actual code does. If something was ambiguous in the docs and could be interpreted either way, it does not count as a breaking change to enforce one of the ways it could be interpreted.
- since the stopping condition would probably be something unrelated to the time
I disagree! And that's the reason time was given as an argument in the first place. I would never use this functionality without an if t < 10000.0 or something condition, to avoid infinite while loops that never stop. But you already said the same thing in the second part of the answer.
Yes, I'm more on the side to keep the current way since if you want to call twice step! with a function, it should be easier to implement a condition related to the number of steps if the time is relative to step!
On the breaking change part I don't agree but the subject is controversial :D : https://dev.to/turnerj/should-behavioural-changes-be-considered-breaking-changes-under-semver-2d5n
So what is your opinion @Datseris on all this? I'm for keeping the time relative when a function is passed for the advantage I stated in the previous comment.
p.s. I can't help on Agents.jl for a while (some weeks) since I'm really short on time with uni work D:
My opinion is the same. Keep the API as is and time is relative to model initial time.
No worries about the lack of time. I'll try to finish the event queue but also no promises.
Yes, I mean that the counter inside step! always starts from 0 each time step is called. It measures time relative to model initial time.