Use SVector for ContinuousAgent

[mastrof]

After I disgracefully deleted the branch I was using for #673, I restored and updated those changes. I could not reopen the original pr, sorry. Closes #672.

Still WIP, I'll try to improve this and then keep updating when needed

Main open points from #673:

• Pathfinder is messy due to multiple SVector <-> NTuple conversions
• Might be better to use typeof wherever possible rather than cast explicitly to SVector

[codecov-commenter]

Codecov Report

Merging #846 (19a6db7) into main (ba92080) will increase coverage by 0.04%. Report is 2 commits behind head on main. The diff coverage is 83.92%.

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
+ Coverage   70.75%   70.80%   +0.04%     
==========================================
  Files          42       42              
  Lines        2773     2788      +15     
==========================================
+ Hits         1962     1974      +12     
- Misses        811      814       +3     

Files Changed	Coverage Δ
src/core/model_abstract.jl	90.76% <ø> (ø)
src/submodules/io/jld2_integration.jl	100.00% <ø> (ø)
src/core/agents.jl	92.30% <66.66%> (-7.70%)	:arrow_down:
src/spaces/walk.jl	88.88% <70.58%> (-0.95%)	:arrow_down:
src/spaces/continuous.jl	93.25% <80.00%> (+0.19%)	:arrow_up:
src/Agents.jl	100.00% <100.00%> (ø)
src/core/model_concrete.jl	89.02% <100.00%> (+0.56%)	:arrow_up:
src/submodules/pathfinding/astar.jl	93.24% <100.00%> (ø)
src/submodules/pathfinding/astar_continuous.jl	93.61% <100.00%> (+0.13%)	:arrow_up:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Datseris]

for me the central point of concern is whether this is breaking or not. It is right? Code that was using continuous agents before this PR will not work with it, right? A trivial MWE would be making a new type of continuous agent inheriting from ContinuousAgent using @agent, and using move_agent! on it to move it to a user specified position.

[mastrof]

Yes, this is breaking right now. It can be surely made non-breaking for any code that was using @agent. e.g. creation

@agent Foo ContinuousAgent{1} begin; end
Foo(; id=1, pos=(1.0,), vel=(1.0,))

works because pos and vel are promoted to SVectors. Also arithmetics mixing NTuples and SVector everything gets promoted to SVector. So with just some minor modifications (like keeping NTuples{D,Float64} in the ValidPos) it should be alright.

The problem is for structs defined manually where pos and vel are hardcoded as NTuple. ContinuousAgent has to explicitly support both SVector and NTuple if we want it to be non-breaking, and eventually we can drop NTuple later when/if you want to move to v6.

[Datseris]

ContinuousAgent has to explicitly support both SVector and NTuple if we want it to be non-breaking.

ContinuousAgent itself isn't an issue here. It is not an abstarct type any ways. As you correctly pointed out @agent will propagate the change to agents made with @agent. By the way, this kind of situation is exactly why we want to make @agent the only supported way to make agents. It is just too messy to let users make agents manually.

The problem is for structs defined manually where pos and vel are hardcoded as NTuple.

Yes, but why, or where, would this be a problem? What we need to do is identify which parts of the code will fail provided that NTUple{Float} is in the ValidPos union. You mention that most operations don't have any problem due to automatic promotion of tuples to SVectors. But I would still like to have a MWE in the test suite to show explicitly what fails and what not.

[mastrof]

You are right, this works without problems (not pushed yet, will test properly and polish in coming days).

julia> mutable struct Foo <: AbstractAgent
           id::Int
           pos::NTuple{1,Float64}
           vel::NTuple{1,Float64}
       end

julia> model = StandardABM(Foo, ContinuousSpace((10,)))
┌ Warning: `vel` field in agent type should be of type `SVector{<:AbstractFloat}` when using ContinuousSpace.
└ @ Agents ~/.julia/dev/Agents/src/core/model_concrete.jl:183
StandardABM with 0 agents of type Foo
 space: periodic continuous space with (10.0,) extent and spacing=0.5
 scheduler: fastest

julia> add_agent!((2.0,), model, (-1.0,))
Foo(1, (2.0,), (-1.0,))

julia> move_agent!(model[1], (1.0,), model)
Foo(1, (1.0,), (-1.0,))

Just had to disable the ArgumentError on model construction and allow pos2cell to accept any ValidPos. I guess we want to throw a deprecation warning for NTuple

[mastrof]

Shall we also change ContinuousSpace to make extent a SVector?

[Datseris]

yes and yes!

[mastrof]

Still need to change ContinuousSpace and remove some explicit SVector conversions here and there. I'll also need to add more tests to make sure that everything works properly for pathfinder and random walks

[Tortar]

I resolved the conflits due to #847 :-)

[mastrof]

I think we should be there, or at least close. Most basic operations seem to work without breakages, both for SVectors and Tuples

[lmiq]

One thing that pops up upon a quick view is if one wants to support any kind of float or only Float64 for the position and velocity fields. That seems, at first sight, contradictory in different files (not that this was introduced in this PR, it was already like this with Tuples)

[Datseris]

It doesn't cost us anything to define ContinuousAgent{D,T} that parameterizes SVector{D,T}. And then define ContinuousAgent{D} = ContinuousAgent{D, Float64}. Could you please add these lines @mastrof and add one very simple test for non FLoat64 numbers?

[mastrof]

Shouldn't you adjust the Flocking example, which in principle should be made much simpler with this PR, as the SVectors can have dot products and everythong else out of the box?

Hmm sorry I fail to see how SVectors change anything here. Also dot (for which I don't see any equivalent in the code) works on tuples too, so.. not sure

[Datseris]

okay sure, but how about the change of parameterizing on the number type T?

[Tortar]

ok not a problem, changing the implementation to account for the new type regains the previous performance :-)

Anyway I think that parametrizing on the float type is a good idea

[mastrof]

@agent ContinuousAgent{D,T} NoSpaceAgent begin
    pos::SVector{D,T}
    vel::SVector{D,T}
end
ContinuousAgent{D} = ContinuousAgent{D,Float64}

This gives LoadError: invalid redefinition of constant ContinuousAgent

@agent ContinuousAgent{D,T} NoSpaceAgent begin
    pos::SVector{D,T}
    vel::SVector{D,T}
end
ContinuousAgent{D}(args...; kwargs...) where D = ContinuousAgent{D,Float64}(args...; kwargs...)

This works, but

julia> @agent Foo ContinuousAgent{2} begin; end

julia> (; zip(fieldnames(Foo), fieldtypes(Foo))...)
(id = Int64, pos = StaticArraysCore.SVector{2}, vel = StaticArraysCore.SVector{2}) 

which means that pos and vel become SVector{2,Float64} only after an agent is created

julia> Foo(; id=1, pos=SVector(2.0,2.0), vel=rand(SVector{2})).pos |> typeof
SVector{2, Float64} (alias for SArray{Tuple{2}, Float64, 1, 2})

julia> Foo(; id=1, pos=(2.0,2.0), vel=Tuple(rand(2))).pos |> typeof
SVector{2, Float64} (alias for SArray{Tuple{2}, Float64, 1, 2})

which I think it's not what we want.

I don't know if there are other options to try

[Tortar]

very good point, I added a commit, now your second solution should work hopefully!

[Tortar]

There is still a problem with how do_checks is implemented since e.g. !(pos_type <: SVector{D,<:AbstractFloat} where {D}) is false and throws a error when one uses a ContinuousAgent just specyfing the dimension, so it should be changed somehow to account for this. Tests fail for this reason, but apart from this it seems working!

[Tortar]

Actually I noticed now that all of this is unfortunately breaking:

e.g.

using Agents
@agent Foo ContinuousAgent{2} begin end
a = Foo(1, (1.0, 1.0), (1.0, 1.0))
a.pos == (1.0, 1.0) # this was true, now it is false since pos is a SVector

However, I fixed the problem with do_checks, now tests should pass.

I would wait till version 6.0 to finally look into this and merge the PR

[Datseris]

Hm... I understand. I think unfortunately with this point @Tortar is right. This should yield a 6.0 release. So far everything in this PR is a-ok however, so it should be merged.

Can someone please clarify what is going on right now in the current source code? If someone inherits

@agent Foo ContinuousAgent{2} begin end

what does (; zip(fieldnames(Foo), fieldtypes(Foo))...) return? Are the fields concretely typed?

[Datseris]

Also, please re-export SVector in this PR.

[Tortar]

yes, they are concretely typed, it now returns (id = Int64, pos = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}}, vel = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}})

Anyway all compatibility changes for NTuple should be dropped since they are not useful since even with them the code is breaking, so it is better not to have them so that the user can actually discover that the fields have a new type.

[Datseris]

yes, they are concretely typed, it now returns (id = Int64, pos = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}}, vel = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}})

????? This is NOT concretely typed. Union types are not concretely typed.

This cannot be allowed, if someone uses just ContinuousAgent{2} in a simulation, it will lead to type instability.

[mastrof]

yes, they are concretely typed, it now returns (id = Int64, pos = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}}, vel = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}})

????? This is NOT concretely typed. Union types are not concretely typed.

This cannot be allowed, if someone uses just ContinuousAgent{2} in a simulation, it will lead to type instability.

This is on the last commit dropping the tuple support:

julia> @agent Foo ContinuousAgent{2} begin; end
julia> (; zip(fieldnames(Foo), fieldtypes(Foo))...)
(id = Int64, pos = SVector{2, Float64}, vel = SVector{2, Float64})

Although on model creation with model(ContinuousAgent{2}, space) you get the warning Agent type is not concrete. Indeed, if you just check on ContinuousAgent{2}, you see the types are not concrete:

julia> (; zip(fieldnames(ContinuousAgent{2}), fieldtypes(ContinuousAgent{2}))...)
(id = Int64, pos = SVector{2}, vel = SVector{2})

edit: Also, honestly I'm not sure how Tortar got that Union? ContinuousAgent was never declared with Tuple types in this branch

[mastrof]

Also, please re-export SVector in this PR.

Since we have to keep StaticArrays anyway for setindex, should I drop StaticArraysCore?

[Datseris]

Since we have to keep StaticArrays anyway for setindex, should I drop StaticArraysCore?

Yes.

This is on the last commit dropping the tuple support:

julia> @agent Foo ContinuousAgent{2} begin; end
julia> (; zip(fieldnames(Foo), fieldtypes(Foo))...)
(id = Int64, pos = SVector{2, Float64}, vel = SVector{2, Float64})

I don't understand. How can it be that inhereting form ContinuousAgent{2} leads to concretely typed fields, but ContinuousAgent{2} itself does not have concretely typed fields...?

[mastrof]

Because ContinuousAgent{D} is parametric, atm we just made it so that its constructor generates a concrete type with the default T=Float64. In the macro we are just adding the Float64 explicitly when it is not declared.

[Tortar]

yes, they are concretely typed, it now returns (id = Int64, pos = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}}, vel = Union{Tuple{Float64, Float64}, StaticArraysCore.SVector{2, Float64}})

????? This is NOT concretely typed. Union types are not concretely typed.

This cannot be allowed, if someone uses just ContinuousAgent{2} in a simulation, it will lead to type instability.

ops, sorry I was attempting a change locally where it was a Union which is of course not concretely typed :D, but here, it was concretely typed because of the macro hack I wrote myself precisely for this, sorry for the confusion

[mastrof]

Just to be clear, given that we are moving to 6.0 anyway, are we completely dropping support for tuples (https://github.com/JuliaDynamics/Agents.jl/pull/846/commits/03e4a009f71b7f82b2e2d4d1f088b5372650f108) or should I revert that given that everything except manual comparison to tuples will work?

[Tortar]

I would say removing the support is better since we are going to a new major version.

A concern though is: manual comparison to tuples is very important, it is obviously something not happening often but it's probably something happened already (e.g. a set of user defined tuples with which compare the position of the agent). The problem is that this is a silent bug since it doesn't throw any error. So we should make the user aware that this doesn't work somehow, I thought about a possible strategy:

• When creating a struct with @agent inhering from ContinuousAgent{D} we warn the user with something along the lines: "ContinuousAgent{Int} is deprecated, use instead ContinuousAgent{Int, :<AbstractFloat}. Be aware that from version 6.0 the position and velocity fields of the agent are of type SVector, so any comparison between these fields and tuples will return false, possibly breaking your past code. Read about this change in version 6.0 at {link to docs}". So in practice we put this warning inside the macro hack.

This is I think very important

[Datseris]

There is no reason to drop support for tuples. This makes user experience better and allows them to use the package while changing minimal code. It makes the transition simpler.

We should just make sure that ContinuousAgent by itself does not include tuples and also that nowhere in the documentation we use tuples for continuous space.

I don't agree with displaying a message in @agent, The macro already has too much hocus pocus. We display an overall update message once the user installs version 6.0, as per usual we have done it like this always in Agents.jl. After all, updating to a new major version means it is up to the user to learn about the breaking changes.

[Datseris]

And we should also never use ContinuousAgent{D} in the docs; only ContinuousAgent{D, Float64}. In the update message ContinuousAgent we will sya that for backwoards compatibility, there is currently a hack that allows agents inhereting from ContinuousAgent{D} to be type stable. In 6.1 this hack will be removed and these agents will be type unstable.

[Datseris]

@mastrof isn't the support for tuples minimal anyways? Most of the code work, you only need to leave tuples in the ValidPos type and take them into account in do_checks, right?

[Tortar]

I'm not particularly fond of the approach, but let's do it that way then!

But I think that @mastrof implemented some more code for retro-compatibility in https://github.com/JuliaDynamics/Agents.jl/commit/03e4a009f71b7f82b2e2d4d1f088b5372650f108 which doesn't seem necessary, I think that @Datseris is right that to keep it we just need adding ntuples to ValidPos and take it into account in do_checks

[mastrof]

With last commit, we should have everything we wanted:

• ContinuousAgent{D,T} is now used everywhere instead of ContinuousAgent{D}
• ContinuousSpace now has extent as an SVector (even if it is passed a NTuple)
• NTuple is accepted for the posand vel fields but throws a deprecation warning so that old code using manually defined agent types should not break
• SVector is reexported

So we will only break code with agents defined by @agent Foo ContinuousAgent where the pos and vel fields are compared for in\equality against tuples.

As per the last comments, right now this backward support is not exactly "minimal", but its not super-complicated either. It's just that there are a few places (e.g. when interfacing with the space extent or with the underlying grid space) where one needs to be careful with Tuple<->SVector conversions, because they happen automatically only one way. It is possible that there are a few unnecessary conversions, but I'm not sure if they can be avoided completely just by spamming ValidPos in all the function signatures.

[mastrof]

* `ContinuousAgent{D,T}` is now used everywhere instead of `ContinuousAgent{D}`

not really I'm still fixing the docs :)

[Tortar]

I removed some transformations but then realized that the consensus is that we should still support also agents manually defined as e.g.

mutable struct Foo <: AbstractAgent
           id::Int
           pos::NTuple{1,Float64}
           vel::NTuple{1,Float64}
       end

so rollbacked the changes, to me it seems good, thanks @mastrof to change back and forth between different versions without giving up :D

I think that it should still be updated the changelog and the message in the Agents.jl file

[Datseris]

@mastrof thanks a lot for your work on this huge pr!

[mastrof]

thank you guys for the help and suggestions :)