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

Allow using const values in agent macro #831

Merged
merged 39 commits into from
Jul 24, 2023
Merged

Allow using const values in agent macro #831

merged 39 commits into from
Jul 24, 2023

Conversation

Tortar
Copy link
Member

@Tortar Tortar commented Jul 14, 2023

This should make it possible to use default values addressing #709 , let me know what do you think. Experimenting a bit, there shouldn't be any perf regression applying @kwdef as default

(I also put in the api page of the documentation the replicate! function, forgot to do that in the other pr :D)

edit: now it also includes the possibility to declare constants, which should help performance, but need some good tests

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023

Codecov Report

Merging #831 (81686fc) into main (de57ea3) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
+ Coverage   70.13%   70.17%   +0.04%     
==========================================
  Files          42       42              
  Lines        2719     2723       +4     
==========================================
+ Hits         1907     1911       +4     
  Misses        812      812              
Impacted Files Coverage Δ
src/Agents.jl 100.00% <100.00%> (ø)
src/core/agents.jl 100.00% <100.00%> (ø)

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

@Tortar
Copy link
Member Author

Tortar commented Jul 15, 2023

Implemented the first way described in #728

using Agents

@agent A NoSpaceAgent begin
    f1::Int
    f2::Int
    consts = (:f1, :f2)
end

agent = A(1, 3, 5)
agent.f1 = 10

returns

ERROR: setfield!: const field .f1 of type A cannot be changed
Stacktrace:
 [1] setproperty!(x::A, f::Symbol, v::Int64)
   @ Base .\Base.jl:38
 [2] top-level scope
   @ REPL[4]:1

@Tortar Tortar changed the title Allow using default values in agent macro Allow using default and const values in agent macro Jul 15, 2023
@Tortar
Copy link
Member Author

Tortar commented Jul 16, 2023

this should be it, all new tests pass!

@Tortar Tortar changed the title Allow using default and const values in agent macro Allow using const values in agent macro Jul 19, 2023
@Datseris
Copy link
Member

does it allow inhereting from a type defined with constants? if we change NoSpaceAgent to have const id::Int, does @agent work with inhereting from NoSpaceAgent?

@Tortar
Copy link
Member Author

Tortar commented Jul 22, 2023

Yes it allows it, any const will be propagated to any inheriting new type, this is so because we actually check if there are any const field in the base type and then add it as a const also in the new type

@Datseris
Copy link
Member

ok great!

src/core/agents.jl Outdated Show resolved Hide resolved
@Datseris
Copy link
Member

Datseris commented Jul 22, 2023

why isn't it possible to do things the native julia way and allow prefacing with const a constant field? Why do we have to have this extra weird consts declaration? what if a user wanted a field named consts?

@Tortar
Copy link
Member Author

Tortar commented Jul 22, 2023

yes, a field name consts is disallowed, there is no way to have const inside the agent macro unfortunately due to parsing errors I think, see JuliaLang/julia#45024

@Tortar
Copy link
Member Author

Tortar commented Jul 22, 2023

it's a bit hacky yes but it's a way not to redefine the agent macro as was done in #728, we could follow that path alternatevely but this means that every code will break :D

@Datseris
Copy link
Member

but the julia issue you cite is fixed...?

@Datseris
Copy link
Member

Okay, I see, there is no way to have const x = 1. Okay. I think your solution with the extra field is the only option then.

But, rename it to constants. and document it appropriately.

@Tortar this is a big change. please do not tag anything after mergfing the const and keyword improvements. i want to read the final docstring and changelog and give the ok for a new tag.

@Tortar
Copy link
Member Author

Tortar commented Jul 22, 2023 via email

@Tortar
Copy link
Member Author

Tortar commented Jul 22, 2023

sure, I actually have another important change which I would like to finish up, we should probably tag them all together

@Tortar
Copy link
Member Author

Tortar commented Jul 23, 2023

@Datseris I completed the higlighting of the changes reverting some of the changed lines in https://github.com/JuliaDynamics/Agents.jl/pull/732/files , I also declared the id field of NoSpaceAgent as constant

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great. Simple comments to address and then please merge (BUT NNOT TAG!!!)

src/core/agents.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@Tortar
Copy link
Member Author

Tortar commented Jul 24, 2023

Merging! (Not tagging :D)

@Tortar Tortar merged commit c3d0426 into main Jul 24, 2023
@Tortar Tortar deleted the kwdef-allowed branch July 24, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants