Skip to content
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

Create a discrete_pos field in ContinuousAgent struct #790

Closed
Tortar opened this issue Apr 2, 2023 · 6 comments
Closed

Create a discrete_pos field in ContinuousAgent struct #790

Tortar opened this issue Apr 2, 2023 · 6 comments
Labels
agent-construction About making agents continuous Continuous space related performance

Comments

@Tortar
Copy link
Member

Tortar commented Apr 2, 2023

Currently, we call pos2cell each time we want to find out what is the position of the agent in the grid behind the ContinuousSpace, this can be improved by creating a new field (something like discrete_pos) automatically set to be equal to the position in the grid, so that we don't recompute the value with pos2cell except when necessary (when the agent moves to a new position). Unfortunately since the creation of Agents uses a macro, in which I don't have much experience, I'm not able to set automatically the discrete_pos field when the agent is created, and clearly it shouldn't be set by the user manually. Doing this will spare some computations for many operations in ContinuousSpace.

@Tortar Tortar added performance continuous Continuous space related agent-construction About making agents labels Apr 2, 2023
@Datseris
Copy link
Member

Datseris commented Apr 2, 2023

Yeap that's a great idea and exactly why we introduced the @agent macro. I wanted to make the usage of @agent mandatory exactly to allow for such optimizations. Unfortunately, due to #709 and #728 I reverted the mandatory enforcement of @agent. But I definitely want to make it the way in the future.

For your suggestion to work, you only need to add this field as a field of ContinuousAgent, and do the necessary code adjustments to update this field when necessary.

You also need to add a check that this field exists in the checks we do.

@Tortar
Copy link
Member Author

Tortar commented Apr 2, 2023

mmh, not so good probably :S

I was too optimistic, given that pos2cell(pos::Tuple, model::ABM) = @. floor(Int, pos/model.space.spacing) + 1 we need the information about spacing when setting the new field at construction time, so it's probably a dead end. Didn't remember the definition very well when I proposed this idea xP.

@Tortar Tortar closed this as completed Apr 3, 2023
@Tortar
Copy link
Member Author

Tortar commented Apr 16, 2023

Actually there is a possible (but breaking unfortunately) way of doing this:

@agent ContinuousAgent{D} NoSpaceAgent begin
    pos::NTuple{D,Float64}
    vel::NTuple{D,Float64}
    space::ContinuousSpace
end

putting a reference inside the ContinuousAgent to the space, tested it and this doesn't degrade performance, but when I do something like

@agent ContinuousAgent{D} NoSpaceAgent begin
    pos::NTuple{D,Float64}
    vel::NTuple{D,Float64}
    space::ContinuousSpace
    discrete_pos::NTuple{D,Int} = @. floor(Int, pos/space.spacing) + 1
end

this fails with

ERROR: LoadError: syntax: "discrete_pos::NTuple{D, Int} =@. floor(Int, pos/space.spacing) + 1" inside type definition is reserved around /home/bob/.julia/packages/Agents/PhazO/src/core/agents.jl:195
Stacktrace:
 [1] top-level scope
   @ none:1
 [2] eval(m::Module, e::Any)
   @ Core ./boot.jl:368
 [3] top-level scope
   @ ~/.julia/packages/Agents/PhazO/src/core/agents.jl:202
 [4] include(mod::Module, _path::String)
   @ Base ./Base.jl:419
 [5] include(x::String)
   @ Agents ~/.julia/packages/Agents/PhazO/src/Agents.jl:1
 [6] top-level scope
   @ ~/.julia/packages/Agents/PhazO/src/Agents.jl:27
 [7] include
   @ ./Base.jl:419 [inlined]
 [8] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt64}}, source::String)
   @ Base ./loading.jl:1554
 [9] top-level scope
   @ stdin:1
in expression starting at /home/bob/.julia/packages/Agents/PhazO/src/spaces/continuous.jl:22
in expression starting at /home/bob/.julia/packages/Agents/PhazO/src/Agents.jl:1
in expression starting at stdin:1
ERROR: LoadError: Failed to precompile Agents [46ada45e-f475-11e8-01d0-f70cc89e6671] to /home/bob/.julia/compiled/v1.8/Agents/jl_DhWG6V.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
   @ Base ./loading.jl:1705
 [3] compilecache
   @ ./loading.jl:1649 [inlined]
 [4] _require(pkg::Base.PkgId)
   @ Base ./loading.jl:1337
 [5] _require_prelocked(uuidkey::Base.PkgId)
   @ Base ./loading.jl:1200
 [6] macro expansion
   @ ./loading.jl:1180 [inlined]
 [7] macro expansion
   @ ./lock.jl:223 [inlined]
 [8] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1144
in expression starting at /home/bob/Desktop/ABM_Framework_Comparisons-main/Agents/Flocking/benchmark_flocking.jl:1

Do you have any suggestion @Datseris on what's need to be updated to make this works? Don't know actually if this will have some notable improvement in performance but I'd like to test if this is the case :-)

(I calculated anyway that the improvement in the grid space iterator + the memory bug resolution made a 4x performance improvement in Flocking!)

@Tortar
Copy link
Member Author

Tortar commented Apr 16, 2023

I actually find some difficulty in not changing the internals and at the same time testing the performance, let me know if you have some suggestion on where the macro can be modified (if it actually can) to make the above redefinition of ContinuousAgent work

@Tortar
Copy link
Member Author

Tortar commented Apr 16, 2023

ok I managed to make it work manually, the difference is very very small, so in the end not worth any change

@Datseris
Copy link
Member

    discrete_pos::NTuple{D,Int} = @. floor(Int, pos/space.spacing) + 1

you can't assign default values inside @agent yet unforutnately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-construction About making agents continuous Continuous space related performance
Projects
None yet
Development

No branches or pull requests

2 participants