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

update to v5.18 #839

Closed
wants to merge 2 commits into from
Closed

update to v5.18 #839

wants to merge 2 commits into from

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented Aug 4, 2023

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2023

Codecov Report

Merging #839 (4b6132d) into main (3ac17b8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #839   +/-   ##
=======================================
  Coverage   70.18%   70.18%           
=======================================
  Files          42       42           
  Lines        2727     2727           
=======================================
  Hits         1914     1914           
  Misses        813      813           

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

@Datseris
Copy link
Member Author

Datseris commented Aug 4, 2023

@Tortar or @AayushSabharwal I am noticing some problems here with checkpointing the shelling example fails when it tries to checkpoint:

https://github.com/JuliaDynamics/Agents.jl/actions/runs/5760638345/job/15616967230?pr=839#step:8:309

i am not sure if this is outdated documentation code, or a genuine bug we haven't written a test for. Can you please help?

@Datseris
Copy link
Member Author

Datseris commented Aug 4, 2023

Meanwhile I have to take care of an error with a file path: https://github.com/JuliaDynamics/Agents.jl/actions/runs/5760638345/job/15616967230?pr=839#step:8:79

@Tortar
Copy link
Member

Tortar commented Aug 4, 2023

will take a look into it soon

@Tortar
Copy link
Member

Tortar commented Aug 4, 2023

What about also removing the outdated comparison table from comparison.md and just keep the link to the repository of the comparisons?

@Datseris
Copy link
Member Author

Datseris commented Aug 4, 2023

yes

@AayushSabharwal
Copy link
Collaborator

Hi! I'm more than happy to take a look, but I'm in the middle of moving and will probably only get time on Sunday.

@Tortar
Copy link
Member

Tortar commented Aug 5, 2023

I'm also thinking that in relation to #841 we should only allow the user to create a new id for an agent with nextid, obviously this is breaking so it's probably not to do on a minor release but we could delete many checks if we do so, and also implementing #841 would ask the user to give control to the library the choice of the id for the new model type. This is something to consider for a 6-version of Agents.jl in my opinion

@Tortar
Copy link
Member

Tortar commented Aug 10, 2023

I thought about a better way to support all possible features we would like (and maybe more) in a better way in the @agent macro than e.g. with special keywords arguments like constants:

  • Allow the standard behaviour of the @agent macro

  • Allow using struct instead of begin in the macro and support all new features here, e.g. const declaration will be done in the standard julian way, which is much cleaner

This is backward compatible, while much better, since it support every feature definable only inside structs (e.g. inner constructors)

@Tortar
Copy link
Member

Tortar commented Aug 10, 2023

I would than wait for this revision for the 5.18

@Tortar
Copy link
Member

Tortar commented Aug 10, 2023

Actually, it is basically what @thevolatilebit described in issue #728 : https://github.com/Merck/AlgebraicAgents.jl/blob/8b7d05a462cd1ee8303fecbf77f01e7fa13b7d07/src/agent_macros.jl#L8 .

We just need to allow also the old syntax not to break people code

@Datseris
Copy link
Member Author

Datseris commented Aug 10, 2023

can you paste the usage code of the new version you propose? I can't imagine it / understand it. Just paste a piece of code that uses this new macro.

@Tortar
Copy link
Member

Tortar commented Aug 10, 2023

mmmh giving more thought indeed it seems that it has not many advantages in addition to a better syntax for const fields, anyway the syntax I would follow is exactly the one described in #728 :

@agent AnotherAgentType [OptionalSupertype] struct YourAgentType{X}
        extra_property::X
        other_extra_property::Int
        const immutable_property::X
        # etc...
end

@Datseris
Copy link
Member Author

well the only difference here is that we have struct instead of begin and that the user-defined type name is last instead of first. It is indeed better to use the native const instead of an extra constants field however. On the other hand, it is better that the name of the thing the user makes is first instead of last. So I don't know.

We can make it non-breaking by naming the new macro @Agent with capital A. which is fine since it makes a type. Old @agent stays as is and thorws a deprecation or something.

@Tortar
Copy link
Member

Tortar commented Aug 11, 2023

I think using @Agent is a good idea, we can even use the following which I think it is a bit better

@agent AnotherAgentType struct YourAgentType{X} [<: OptionalSupertype] 
        extra_property::X
        other_extra_property::Int
        const immutable_property::X
        # etc...
end

@Datseris
Copy link
Member Author

okay, can you make this work as well as @agent does now? And does this address the problems @mastrof has in #725 ?

@Tortar
Copy link
Member

Tortar commented Aug 11, 2023

yes, surely we can make it work the same way, #725 should be already addressed in #843 anyway

@Datseris
Copy link
Member Author

i forgot i had otifications off for this repo.

Okay, let's please re-calibrate ourselves. Is there any reason to introduce @Agent now that @agent achieves everything? What do we gain besides the slightly better syntax?

I guess we can add both and say in @agent that we provide this only as a transition to @Agent which will be the only one available in Agents v6+ ?

@Tortar
Copy link
Member

Tortar commented Aug 11, 2023

I totally agree with the transition phase, I think, apart from the better syntax, we get a more elusive benefit but I think anyway good for the future of the package: if more struct-only keywords will be supported we can easily support them too without the strange deviations from usual syntax, see this issue https://github.com/JuliaLang/julia/issues/35795 for new syntax inside for the fields of a struct which can eventually be supported in Julia

@Datseris
Copy link
Member Author

Datseris commented Aug 11, 2023

okay the discussion here has deviated dramatically from the original purpose (which was to fix the issue in the documentation builds in schelling that I cite above: #839 (comment) )

I would say, go ahead and impleemnt @Agent after merging in #843 and tag me there. Once @Agent is in, I'll re-adjust this PR to update the docs accordingly.

@Tortar
Copy link
Member

Tortar commented Aug 11, 2023

anyway I ran https://github.com/JuliaDynamics/Agents.jl/blob/main/examples/schelling.jl and there isn't any problem with checkpointing. I think the cause is outdated documentation

@Tortar
Copy link
Member

Tortar commented Aug 11, 2023

I'm asking myself where documenter.jl saves the file when the line AgentsIO.save_checkpoint("schelling.jld2", model) is run? Maybe it can't find the saved file and so model = AgentsIO.load_checkpoint("schelling.jld2"; scheduler = Schedulers.Randomly()) fails? the traceback seems something else though

@AayushSabharwal
Copy link
Collaborator

AayushSabharwal commented Aug 13, 2023

I've spent some time on this, but Documenter provides no context for the error. I suspect it's something inside JLD2, since that's the only place I can find .parameters . TypeName seems to be Core.TypeName, which is created by doing .name (or calling Base.typename) on a type (e.g. Dict{Int,Int}.name or Base.typename(Dict{Int,Int})).

It's also probably a good idea to fix this warning:
image

@Tortar
Copy link
Member

Tortar commented Sep 6, 2023

Meanwhile I have to take care of an error with a file path: https://github.com/JuliaDynamics/Agents.jl/actions/runs/5760638345/job/15616967230?pr=839#step:8:79

I solved this in #867

@Datseris
Copy link
Member Author

Datseris commented Sep 8, 2023

@Tortar can you please update this to current main and merge it in? It has become difficult to track the status quo, and the main changelog should be the best representative of what is going on.

@Tortar
Copy link
Member

Tortar commented Sep 25, 2023

can we close this one given that we are going to 6.0? Probably we need to open an issue for the Schelling checkpoint problem with Documenter.jl

@Datseris Datseris closed this Sep 25, 2023
@Datseris Datseris deleted the release_v5.18 branch September 25, 2023 12:10
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.

4 participants