JuliaDynamics / Agents.jl

Agent-based modeling framework in Julia
https://juliadynamics.github.io/Agents.jl/stable/
MIT License
725 stars 117 forks source link

Agent macro field inheritance #885

Closed mastrof closed 11 months ago

mastrof commented 11 months ago

Extends the @agent macro to inherit default values of base types. Closes #844.

codecov-commenter commented 11 months ago

Codecov Report

Merging #885 (ff378d1) into main (71fac47) will increase coverage by 7.88%. Report is 11 commits behind head on main. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #885      +/-   ##
==========================================
+ Coverage   72.72%   80.60%   +7.88%     
==========================================
  Files          42       45       +3     
  Lines        2871     3022     +151     
==========================================
+ Hits         2088     2436     +348     
+ Misses        783      586     -197     
Files Coverage Δ
src/Agents.jl 100.00% <ø> (ø)
src/core/agents.jl 100.00% <100.00%> (ø)

... and 34 files with indirect coverage changes

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

mastrof commented 11 months ago

With https://github.com/JuliaDynamics/Agents.jl/pull/885/commits/2f5c0e0089582642578769f1b79d90e32359d1d6 I commented out the Models submodule because the macro is not yet working with calls of the form @agent Foo(GridAgent{2}). The other base agent types are correctly generated on loading Agents.jl and their expressions stored as

julia> __AGENT_GENERATOR__
Dict{Symbol, JLKwStruct} with 5 entries:
  :GraphAgent      => begin mutable struct GraphAgent <: Agents.AbstractAgent; $(Expr(:const, id::…
  :GridAgent       => begin mutable struct GridAgent{D} <: Agents.AbstractAgent; $(Expr(:const, id…
  :OSMAgent        => begin mutable struct OSMAgent <: Agents.AbstractAgent; $(Expr(:const, id::In…
  :NoSpaceAgent    => begin mutable struct NoSpaceAgent;  $(Expr(:const, id::Int)); end; begin fun…
  :ContinuousAgent => begin mutable struct ContinuousAgent{D, T} <: Agents.AbstractAgent; $(Expr(:…
mastrof commented 11 months ago

Also noticed that with the new form of the macro (#870) not even inheritance from parametric types is working currently, which instead was working trivially with the old macro version. This is due to the bad parsing that I do when collecting the name of base type.

Tortar commented 11 months ago

Will clone the branch soon and try to solve the problems you are mentioning!

Tortar commented 11 months ago

mmmh there is actually a problem we didn't identify when considering the alternative which I think it could be unsolvable with this methodology, parametric types can't work this way, take

abstract type Foo end
@agent struct NewAgent(GridAgent{2}) <: Foo
    x::Int = 1
    const y::Int
end

this is not in __AGENT_GENERATOR__, only GridAgent{D} is. This means that we can't simply do all the work with ExproniconLite, but we should first evaluate GridAgent{2} to obtain its specific fields and then rebuild the expression with the defaults (sorry GridAgent{2} has no defaults, but this applies anyway for a parametric user-defined agent with defaults), which is trickier...however it is not that simple since consider if patologically the user-defined agent is actually

abstract type Foo end
@agent struct NewAgent{D}(GridAgent{2}) <: Foo
    x::Int = D
    const y::Int
end

@agent struct AnotherAgent(NewAgent{2}) <: Foo
    z
end

then the default should not be only retrieved but even substituted back before returning the expression for AnotherAgent which is rather tricky.

So I think that ExproniconLite is actually not helping us much in these regards, and maybe the simpler @capture from MacroTools could be enough, but the actual implementation is not obvious (even if possible)

Tortar commented 11 months ago

I think a possible alternative strategy is to reconstruct only the function definition with default values which @kwdef creates macro-expanding it, this should be doable

mastrof commented 11 months ago

I also eventually came to the conclusion that Expronicon is not the right tool for the job. @macroexpanding kwdef to extract constructor sounds like a good option at this point, I guess the main problem then becomes to correctly parse and substitute the type parameters

Tortar commented 11 months ago

ok, I think I got a good representation of what we need to achieve:

Trasform this

@kwdef mutable struct A{D}
    x::NTuple{D, Int} = Tuple(x for x in 1:D)
    y::Int
end

@kwdef mutable struct B{K}
    z::NTuple{K, Int} = Tuple(x for x in 1:K)
    u::Int
end

into

@kwdef mutable struct B{D, K}
    x::NTuple{D, Int} = Tuple(x for x in 1:D)
    y::Int
    z::NTuple{K, Int} = Tuple(x for x in 1:K)
    u::Int
end
B{K}(; kwargs...) where K = B{2, K}(; kwargs...) # example to support specified parameters for base type
Tortar commented 11 months ago

mmmhh I don't think what I proposed is optimal, it is probably better to go with the current way(even if I would actually avoid Expriconlite and go with MacroTools) and substitute the parameters when e.g. we saved in the dict GridAgent{2} but we have GridAgent{D} in the dict as described in https://stackoverflow.com/questions/51928959/how-to-find-and-replace-subexpression-of-ast-in-julia

I still didn't have a look at your latest commits, maybe you are already trying to do this :D

Tortar commented 11 months ago

Good news: made it!!!

(But still very unpolished)

This should be actually even much better than #843 e.g. this now works:

using Agents
abstract type AbstractFoo{D} <: AbstractAgent where D end
@agent MyFoo2{D}(ContinuousAgent{D, Float64}) <: AbstractFoo{D} end

No need for where inside the macro, and the parameters are passed better than before since before if you called MyFoo2{2} the types were still loosy like pos::NTuple{D, Float64} where D while now are like pos::NTuple{2, Float64}.

We even finally avoid eval!

Tortar commented 11 months ago

it seems to me that the failures are actually unrelated, can you update the branch?

edit: wops, it was a little bug

Datseris commented 11 months ago

this was merged without a changelog entry even though it was a new feature and not a bugfix. In the future please ensure that merged PRs have some accompanying documentation if they introduce new features.

mastrof commented 11 months ago

I am really amazed by your macro wizardry.