-
Notifications
You must be signed in to change notification settings - Fork 940
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
A unified interface for different collection types #2671
Comments
I agree with that. We will use a similar collection implementation to manage layers, regular agents and geo-agents (e.g., with vector geometry). When refactoring |
Do we need a full class hierarchy for this, or can we use duck-typing instead? That is, it might be easier to agree on a shared interface across the collection types and standardize this as a protocol |
protocol vs. generic probably depends on the amount of re-usable code between sub-types, such as between Lines 200 to 246 in ba5104f
and mesa/mesa/experimental/cell_space/cell_collection.py Lines 113 to 145 in ba5104f
Using inheritance might help reduce these duplicated code. On a side note,
If this is the case, then we need to change the definition of class CellCollection(Generic[T], CollectionBase[T]):
"""Cell-specific collection implementation"""
pass |
|
Thanks for the response!
This isn't really an issue for me -
much like how agents are owned by a model: Line 108 in 8c24e1b
Similarly layers are owned by
Then we can have the performance improvement for all collections.
In this case probably I'm not entirely sure about this though. Anyways even if we'd like to make the change, this could be in a separate issue/PR? Any comments/ideas on this issue will be much appreciated! @EwoutH @Corvince |
I don't follow your response to point 1. The use of weakrefs is a deliberate implementation choice in |
Hi @quaquel, @wang-boyu, and @SongshGeo! Thank you for your comments. It seems we have reached a consensus on enhancing the collection, but there is a point of disagreement on whether to use duck typing as a protocol or the CollectionBase class. I am considering three possible approaches:
I have already implemented the approach 3, and I will also implement the other two approaches. I think we only need to run some benchmark experiments to compare the performance of these three approaches. I will proceed to complete these experiments, and we can decide on the next steps based on the results. If you have any suggestions or guidance regarding the benchmark experiments, please let me know! |
@quaquel already got it right, but yes DiscreteSpace allows using |
I would lean towards protocol, although I agree it depends on how similar the code is. Select seems pretty similar, but there are already small differences (inplace), so I am unsure if we can really consolidated every use case in a single method. Might get ugly around the corners. Duck typing keeps things nicely separated, where differences are present (already visible with weakref/no-weakref). Code can also be duplicated by calling a shared function. |
Thanks @Corvince. It would probably be easier to try duck typing (e.g., through I'm thinking perhaps it's possible to have a generic |
I like this pragmatic way forward. Let's standardize and formalize the interface first via a protocol. That should be pretty straightforward because I have tried to keep CellCollection and AgentSet similar and have ported api ideas from one to the other. |
What's the problem this feature will solve?
Currently, Mesa has multiple collection implementations (
AgentSet
,CellCollection
) with overlapping functionality. This leads to:Describe the solution you'd like
Introduce a new
CollectionBase
class that would:Provide a common foundation for all Mesa collections with shared functionality:
__contains__
,__len__
)Use weak references for proper memory management and garbage collection, crucial for simulation performance
Example usage:
Benefits:
Additional context
This would be a significant architectural improvement for Mesa, making it more maintainable and extensible while providing a better developer experience.
The text was updated successfully, but these errors were encountered: