-
Notifications
You must be signed in to change notification settings - Fork 936
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
Automatically assign unique_id
to Agents
#2226
Conversation
Performance benchmarks:
|
This is an interesting approach, thanks! Do you mind if I write some test cases and add them to this PR? Then we can see what edge cases are covered and for which we still have to come up with a solution. |
Good stuf. A few comments that might be useful to move this PR forward
|
sure! |
Done! I added 3 test cases, as you can see, currently one of them fails. What helps is that most people just pass The test that currently fails is the one that uses all keyword arguments: super().__init__(unique_id=unique_id, model=model) Maybe Curious if you can get all three test cases simultaneously working! |
@EwoutH here are the code changes I am making:
All tests are passing expect the tests in
My code changes checks only for instance So, can strings be passed as arguments for a unique_id? To solve this I also tried to do I can refactor the code to not include any type checking for |
Also Instead of adding another optional paramter |
I don't see a simple solution for supporting both current style and new style Agents. Currently, For my point 2 above, see the SimulationEvent code on how to handle the counting nicely. The code there is the recommended way of counting instances of a class on StackOverflow. This can be implemented in a backward compatibly way and then just let |
I thought it would break a lot more models, but since the name of the variable in your subclass are defined by yourself and the variables names only matter when calling The fact that all regular example models passed makes it a lot less impactful to make a breaking change here. |
mesa/agent.py
Outdated
@@ -35,16 +35,26 @@ class Agent: | |||
self.pos: Position | None = None | |||
""" | |||
|
|||
def __init__(self, unique_id: int, model: Model) -> None: | |||
def __init__( | |||
self, unique_id_or_model: int | Model, model: Model | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make it this:
self, unique_id_or_model: int | Model, model: Model | None = None | |
self, model: Model, fallback: None = None |
And then warn if either model
is not a Model instance or if fallback
is not None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GittyHarsha what do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to support existing codes? If so then keywordAgent cannot be supported?
Sorry, I didn't understand the purpose of fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the idea.
The purpose of the fallback is to not let models that use two positional arguments (which is very common) crash hard.
So:
We support and recommend:
super().__init__(my_model)
We allow
super().__init__(unique_id, my_model)
which we convert internally to to let my_model be the model and throw unique_id away, with a warning to convert to super().__init__(my_model)
.
I think this will be a good balance to moving towards the future of Mesa and not breaking existing models too hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self, model: Model, fallback: None = None
according to this, the order of arguments should be (model, unique_id) but existing codes use the order (unique_id, model).
Also is there a need to support keyword arguments? In that case, this doesn't work I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so we have to handle that internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this is a good alternative.
To handle keyword arguments shall we handle it via **kwargs?
self, model: Model, fallback: None = None, **kwargs
And check if the unique_id
keyword is present and handle it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented your suggestions:
_ids = itertools.count()
def __init__(
self, model: Model, fallback: None = None, **kwargs
) -> None:
if "unique_id" in kwargs:
self.unique_id = kwargs["unique_id"]
self.model = model
elif fallback is None:
self.unique_id = next(self._ids)
self.model = model
else:
self.unique_id = model
self.model = fallback
only one test is failing
super().__init__(unique_id, model=model)
E TypeError: Agent.__init__() got multiple values for argument 'model'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I don't care too much about that test case, it won't happen too often.
I do want to use the next(self._ids)
always, so just ignore the unique_id inputted by the user.
this passes all the tests. |
Do you mind if I make some modifications on your branch? You made a great start but I would like to tidy it up myself. |
Sure! No problem. I learnt a lot trying to solve this issue, especially as someone new to open source |
3ace802
to
6e383cf
Compare
Co-Authored-By: HarshaNP <96897754+gittyharsha@users.noreply.github.com>
6e383cf
to
a49ed23
Compare
They assumed starting at 0, we now start at 1
4236d74
to
d6ffcac
Compare
@projectmesa/maintainers Ready for review! |
Performance benchmarks:
|
self.current_id += 1 | ||
return self.current_id | ||
return return_id | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my opinion, this is part of the behavior of the Agent class so it is strange that it is located in the model class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but let's move it in a separate PR. It breaks practically all tests with more than one test class or function in a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I fully understand your point. Why not move the counting code into Agent in this PR?
I haven't looked at how bad this would be in terms of tests, but it seems logical to include it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy my branch and move it. You will see what happens, and maybe you have an elegant solution for it.
"The model parameter is required to initialize an Agent object. Initialize the agent with Agent(model, ...)." | ||
) | ||
|
||
self.unique_id = self.model._next_id | ||
self.pos: Position | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to have a closer look tomorrow. I think it might be possible to ensure backward compatibility in a more elegant way.
The benchmarks are slower because they still use the old structure which creates a huge amount of warnings. |
If they stop throwing warnings, it goes probably faster.
a1e7cc7
to
002b5d9
Compare
This might actually be the largest breaking change we make in Mesa 3.0. I hope it's worth it. There was some beauty in explicitly knowing that you assigned each agent and unique ID. It was one of the first things you had to teach. It might get used a lot less when that isn't automatic anymore. I still dislike having the example models in two places. It's maintenance intensive. |
4d86d9d
to
8e7199e
Compare
Performance benchmarks:
|
A few comments from my side (open for discussions):
|
Regardless of how it is done, it should move the Agent class. I am not familiar with
If you make it part of MESA, uniqueness is guaranteed by construction. Messing with internal attributes of classes is at your own peril. So I don't see a need to be so defensive in how we code it up. If we want to be defensive, you could move |
First of all lets not again discuss this PR to its death. Great work by @GittyHarsha and @EwoutH. I think making it automatic by default is very good and the main motivation for this PR. Everything else can be discussed separately.
I did the same, but I think its overkill. UUID is often used to make the ids universally unique and to prevent guessing the next id. We don't need either of those. As a side effect a counter even conveys additional information, for example the order in which agents were created. Its also much easier for humans to track a number than a uuid.
I think its unnecessary. For us maintainers we now know for certain that the unique_id is truly unique. Before this PR I was always reluctant to fully rely on this.
Again, we don't need this if unique_id is not, but automatically assigned.
Partly agree. At first I thought we should move it to the |
I would be fine with that as well. In SimulationEvent, I used a class based approach based on the most upvoted StackOverflow answer, but I am fine either way. The main point for me is that it is not part of the behavior of |
I just want to add: I try to keep my PRs small in scope and if possible even atomic. I know there's benefit into doing things right once, but sometimes that makes PRs unmanageable big, either code wise or, more often, conceptually (which is why we don't have a new datacollector yet). This PR modifies one thing: Automatically increasing the
So again, I'm not fundamentally against any of these changes. It just are more changes that are better fitted in a new PR. |
Thanks for your responses @quaquel @Corvince! Some further comments - Regarding implementation details of I don't really care if it's using
I wouldn't count on these because agents can be removed/destroyed anytime. We then need to differentiate how many agents were created and how many agents are still alive.
I agree. But if it's using a counter, then the counter has to be stored somewhere outside individual agents. Keeping it inside a model makes sense. However, this is also why I think using Regarding user-settable The central question is: why are users not allowed to use their own unique ids? My suggestions are:
As an example, Mesa-Geo sets |
I ran a quick performance test on just 1000 agents, UUID vs. itertools.count I just don't see the need for UUID in this context, and it comes with a real performance difference.
I agree but there is @EwoutH point on how this causes all tests to fail. I want to take a closer look myself at this later today. I think it is possible to move everything into
In my screenshot,
This is perfectly analogous to the discussion on automatically counting steps. Users can still do this, but just not by using the unique_id attribute. MESA relies on unique_id for agent data collection, so it makes sense that MESA internally handles this correctly. As a minor point, it was deeply confusing to many of my students why they had to supply the unique_id themselves. |
@EwoutH, you might want to check #2260. This is how I would go about solving the issue. I don't have any existing tests failing, while unique_id counting is done inside the Agent class. #2260 is, of course, not complete. For example, tests are still needed, and I would add a deprecation warning inside |
@Corvince you’re spot on. That’s what I discovered when I tried to move it. That’s where all the tests were failing. That’s why I didn’t move it. And I explained this I think 5 times now. Sorry guys, but I’m getting a bit done with the “whataboutism” this way we never get anything done. We’re all experienced developers. We have to trust each other and have to give each other a bit of room. If you really care that much open a follow-up PR yourself. |
Just for the record, my comments are not blocking. I remembered we discussed this before (correct me if I'm wrong), if at least two maintainers approve a PR then it's considered mergable. |
I’m resigning as owner of this specific effort. Feel free to work further on this branch. |
Isn't this contradictory to the rest of your argument? ;) If this is the case, and I basically agree with this, then why make so much fuss about being able to change them? If it's just for identification, one can be happy that it happens automatically, no need to assign custom names or anything else to them. What I meant with positive side effects that you can eyeball some information from them. Nothing to rely on, but a good estimate about the age of the agent and if you are tracking the same agent in multiple places |
makes this consistent with projectmesa#2226
makes this consistent with projectmesa#2226
This PR ensures that the unique_id in Agent is assigned automatically. It draws on work done in #2226. In short, this PR makes 3 changes: Agent.unique_id is assigned automatically and no longer used as an argument to Agent. A deprecation warning is issued if the Agent is instantiated with 2 arguments (unique_id and model). The value passed for unique_id is ignored in favor of the new system. This last point is in line with Automatically assign unique_id to Agents #2226. Model.next_id() raises a deprecation warning and always return 0. unique_id is unique relative to a Model instance. Internally, Agent has a class-level attribute _ids. This attribute is a dictionary with the Model instance as key and the "generator" for unique_id as value. All unit tests and benchmarks work with the new structure. They raise no deprecation warnings. examples, tutorials, etc., still need to be updated to be consistent with this change.
closing as solved via #2260 |
Automatically assign
unique_id
to Agents. This has two goals:unique_id
value on creation.unique_id
values are actually unique.A few design considerations:
current_id
counter starts now at0
, instead of1
.for i in range(n)
loops, which also assign unique_ids from 1.next_id()
was commonly use, so that also gets a deprecation warning.current_id
counter is kept public. It can be useful to see how many agents are created in total, and is used here and there. Again, can be modified in another PR.Usage
Instead of:
You can (and should) now initialize your Agent with:
Resolves #2213