JuliaDynamics / Agents.jl

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

Use `@dispatch` instead of branching for multiagent models #1034

Closed Tortar closed 3 weeks ago

Tortar commented 1 month ago

Still a bit experimental in MixedStructTypes, so this still needs some adjustments, but I'd like to know what you think about this @Datseris (I'm still a bit unsure about the name of the macro). In practice what it does is to reconstruct the branching for the user. Actually in some more advanced cases, it could be more complicated to write the branching, with this it should be a lot easier.

Datseris commented 1 month ago

so what's the documentation? If I am a user wanting to learn, what do I read?

Tortar commented 1 month ago

Currently, there is a section in the ReadMe in MixedStructTypes describing it (https://github.com/JuliaDynamics/MixedStructTypes.jl?tab=readme-ov-file#define-functions-operating-with-the-mixed-structs) , but needs to be expanded.

In Agents.jl we probably need to add it to the tutorial when describing the multiagent macro.

Datseris commented 1 month ago

Okay, so this is a non-breaking addition that allows using a "multiple dispatch" like syntax for @multiagent. That seems totally fine for me. How aobut you give a shot at documenting this in the tutorial and we see how the final producct looks?

Tortar commented 1 month ago

Maybe this is good enough? I will wait a little bit before merging anyway because I need still to improve a bit more the macro

Datseris commented 1 month ago

I don't think it is a good idea to go all in into the @dispach. In general, keep in mind, macros are a concept that really only Julia uses from mainstream languages. Users coming from Java, C, Python, R, don't really use macros in typical user code. I think it is best if in the tutorial we first show the non-macro versions for everything, and then show the macro version and say "look, the macro version is created so that @multiagent is as harmonious as standard Julia multiple dispatch". (although, itself MD is also advanced concept that only Julia uses)

Tortar commented 1 month ago

Yes, I think you are right, I just demonstrated the branching version before the macro one now in the event-based tutorial. Now it should be good to go...but actually I still need to register the package so we need to wait a little bit more :D

Datseris commented 1 month ago

register which package? edit: okay saw the rename.

Tortar commented 4 weeks ago

The package was released, so now it is ready

Datseris commented 4 weeks ago

"pattern" is a name that is never used in the Julia language as far as I can tell. Why introduce new terminology? I much prefer dispatch instead, which parallelizes actual multiple dispatch, which is the whole goal here.

Tortar commented 4 weeks ago

it's actually more sensible here because it's a way to express pattern matching

Tortar commented 4 weeks ago

another package using it (defunct but maybe cool to resurrect) https://github.com/toivoh/PatternDispatch.jl and there are many others in this space using this terminology

Datseris commented 4 weeks ago

it's actually more sensible here because it's a way to express pattern matching

I disagree. What we are doing here is providing a convenience syntax for multiple dispatch. Can you argue that we do not? Or, can you provide arguments why this name shouldn't be @dispatch? Using @dispatch as a name does not require adding additional names, as we have already discussed dispatch in the tutorial for the subtyping agent approach.

As far as I can tell, the only reason to introduce here this new syntax is to allow multiple dispatch like syntax for the multiagent, so people don't have to write if clauses with kindof. So...

As for "pattern matching", the only context I've heard pattern matching is in strings and regexes. The link you shared is something entirely different. That is an argument that having basic familiarity with Julia, or Agents.jl, does not mean you have familiarity with what "pattern matching" is. That's not even my worst gripe with the link you shared. They use exactly the same "function" @match to do both search as well as manipulation/transformation of data. Based on the principles of good code I teach often, this goes against having one function do "one thing".

Datseris commented 4 weeks ago

another package using it (defunct but maybe cool to resurrect) https://github.com/toivoh/PatternDispatch.jl and there are many others in this space using this terminology

First, we can't use an 8 years old package that is more abandoned than anything else in the ecosystem to guide any decision. But even if we did, this package would favor using @dispatch instead of @patter which is what I argue...

Tortar commented 3 weeks ago

I would just like to indicate a bit that what it is really happening is not dispatching, the correct name is anyway pattern matching, look at the Rust docs for another example https://doc.rust-lang.org/book/ch06-00-enums.html. I had another name for the macro which I think it's okay which is @branch, I'm not sure that this is better though, because it doesn't have any good reference.

That said, if you feel strongly that @dispatch is better, we could just create a simple indirection in Agents.jl, while mantaining the original name in DynamicSumTypes.

Datseris commented 3 weeks ago

Yes, I understand that we are not doing multiple dispatch. We are disguising if clauses so that they look like multiple dispatch. That us why @dispatch is a name I believe fits best. Macros are about looks after all, all they do is disguise some code (formally, re-write it). @branch is a name I can also accept, if you prefer that one.

Datseris commented 3 weeks ago

let me think about this for a while before merging anything.

Tortar commented 3 weeks ago

Actually...I found a much simpler methodology for all of this, which let us use real dispatch without any problem, honestly I feel a bit bad because it took to me so many hours to write the DynamicSumTypes package and it is actually useless, because a so much easier methodology is possible, I will write it down in as soon as I find the time, so let's put this on the waiting list for some more time

Datseris commented 3 weeks ago

Would be good to leave this PR as is, as things here are working fine. After thinking a lot, I believe @dispatch is the best name for this particular case.

Tortar commented 3 weeks ago

fortunately I checked this other implementation I mentioned, and it doesn't work as well as I was expecting, so I think this is actually very good as it is.

As for the name, then I will create a simple indirection in Agents.jl because I'd like to keep pattern in DynamicSumTypes

Tortar commented 3 weeks ago

should be okay now

Datseris commented 3 weeks ago

Hey @Tortar, while reviewing your PR, I'd suggest the following code changes:

👉 Code Suggestion for #1034

https://github.com/JuliaDynamics/Agents.jl/pull/1034

You can also review and apply these suggestions locally on your machine.

Learn more about GitKraken Code Suggest >[Code Suggest](https://gitkraken.dev/redirect/aHR0cHM6Ly93d3cuZ2l0a3Jha2VuLmNvbS9zb2x1dGlvbnMvY29kZS1zdWdnZXN0P3NvdXJjZT1naXRodWIuY29tJm1lZGl1bT1wcl9jb21tZW50?source=PRSuggest&event=redirectToCodeSuggestPage&provider=github) liberates your code reviews from GitHub's restrictive, comment-only feedback style. As simple as suggesting changes in a Google-doc, provide real code suggestions from where you code, e.g. your IDE, and on anything in your project — not just on the lines of code changed in the PR. _Join your team on [GitKraken](https://gitkraken.dev/redirect/aHR0cHM6Ly93d3cuZ2l0a3Jha2VuLmNvbT9zb3VyY2U9Z2l0aHViLmNvbSZtZWRpdW09cHJfY29tbWVudA==?source=PRSuggest&event=redirectToIndexPage&provider=github) to speed up PR review._
Datseris commented 3 weeks ago

haha okay i tried to do something fancy with gitkraken but it requires you to log in stuff. I'm pushing here my changes.

Tortar commented 3 weeks ago

I looked at them, and everything is okay apart from an explaination about the macro which was not really that accurate, so I changed it in this commit: https://github.com/JuliaDynamics/Agents.jl/pull/1034/commits/c83476a0a5a89f7f37c865d344bfa37654479361.

Didn't know about GitKraken, seems interesting but yes it requires some login I don't have really fast access to :-)

Datseris commented 3 weeks ago

yes, thanks for clarifying I Didnt know