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

Use @pattern instead of branching for multiagent models #1034

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

Tortar
Copy link
Member

@Tortar Tortar commented May 11, 2024

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
Copy link
Member

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

@Tortar
Copy link
Member Author

Tortar commented May 12, 2024

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
Copy link
Member

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
Copy link
Member Author

Tortar commented May 12, 2024

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
Copy link
Member

Datseris commented Jun 2, 2024

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
Copy link
Member Author

Tortar commented Jun 2, 2024

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
Copy link
Member

Datseris commented Jun 2, 2024

register which package? edit: okay saw the rename.

@Tortar Tortar changed the title Use @dispatch instead of branching for multiagent models Use @pattern instead of branching for multiagent models Jun 8, 2024
@Tortar Tortar marked this pull request as ready for review June 8, 2024 14:12
@Tortar
Copy link
Member Author

Tortar commented Jun 8, 2024

The package was released, so now it is ready

@Datseris
Copy link
Member

Datseris commented Jun 8, 2024

"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
Copy link
Member Author

Tortar commented Jun 8, 2024

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

@Tortar
Copy link
Member Author

Tortar commented Jun 8, 2024

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
Copy link
Member

Datseris commented Jun 8, 2024

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
Copy link
Member

Datseris commented Jun 8, 2024

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
Copy link
Member Author

Tortar commented Jun 8, 2024

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
Copy link
Member

Datseris commented Jun 8, 2024

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
Copy link
Member

Datseris commented Jun 8, 2024

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

@Tortar
Copy link
Member Author

Tortar commented Jun 9, 2024

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
Copy link
Member

Datseris commented Jun 9, 2024

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
Copy link
Member Author

Tortar commented Jun 9, 2024

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

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.

None yet

2 participants