mesa icon indicating copy to clipboard operation
mesa copied to clipboard

Automate and enforce unique `unique_id` in Agents

Open EwoutH opened this issue 1 year ago • 13 comments

Currently, Agent's unique_id values are assigned manually on Agent creation, and not guaranteed to be unique. This issue:

  • Automatically assign agents an unique_id value on creation.
  • Guarantee that all unique_id values are actually unique.

EwoutH avatar Aug 15 '24 13:08 EwoutH

Hi @EwoutH here is my initial proposal: change the __init__ function of Agent class to the below:

def __init__(self, model: Model) -> None:
        """
        Create a new agent.

        Args:
            model (Model): The model instance in which the agent exists.
        """
        self.unique_id = model.next_id()
        self.model = model
        self.pos: Position | None = None

        # register agent
        try:
            self.model.agents_[type(self)][self] = None
        except AttributeError as err:
            # model super has not been called
            raise RuntimeError(
                "The Mesa Model class was not initialized. You must explicitly initialize the Model by calling super().__init__() on initialization."
            ) from err

Basically, I used next_id() member of Model class as it maintains current_id: A counter for assigning unique IDs to agents.

GittyHarsha avatar Aug 19 '24 09:08 GittyHarsha

I think this is quite clean!

We could even make this less breaking by making model.next_id a property, and then setting that as the default value for agent.unique_id, so you can overwrite it if you want to, but also it doesn't break current builds.

@quaquel @rht another thought of me was maybe handeling this as an Agent Class attribute. What's the way you would implement this?

EwoutH avatar Aug 19 '24 09:08 EwoutH

In my view, the counter stuff should be in the Agent Class not the Model class. There are some metaprogramming things possible depending on how we want to handle it. Do we want a single counter across all Agent subclasses or count seperately for different subclasses? I don't think a property is required nor is users overwriting this desirable since it might brake other parts of MESA that depend on unique_id (e.g., some future version of data collection).

quaquel avatar Aug 19 '24 10:08 quaquel

@quaquel

In my view, the counter stuff should be in the Agent Class not the Model class

in this case, how can the agent IDs be unique in a particular model?

GittyHarsha avatar Aug 19 '24 10:08 GittyHarsha

Do we want a single counter across all Agent subclasses or count seperately for different subclasses

I was also thinking about this, I'm not sure what's better. I'm nudging towards the former.

users overwriting this desirable

Agreed, users can define other things if they want.


I don't want to break every Mesa model currently in existence in a hard way.

There are two cases we have to look out for:

  • Agents defined with positional arguments (MyAgent(i, self))
  • Agents defined with keyword arguments (MyAgent(unique_id=i, model=self))

Both should be let down gracefully (a warning for now), but also with a clear migration path to what to do next.

@GittyHarsha sorry, this issue is probably a bit more complicated then I thought.

EwoutH avatar Aug 19 '24 10:08 EwoutH

Actually I think it’s quite doable, with some type checking on the input arguments. @GittyHarsha do you want to give it a shot with your current proposal? If so, feel free to open a draft PR!

Even if we go in another direction it will be useful to have figured out how to handle warnings for current models.

EwoutH avatar Aug 19 '24 10:08 EwoutH

I was also thinking about this, I'm not sure what's better. I'm nudging towards the former.

me too

I don't want to break every Mesa model currently in existence in a hard way.

The simplest solution is to issue a warning once in the next release of MESA 2, while properly rerigging everything in MESA 3.

quaquel avatar Aug 19 '24 10:08 quaquel

I was going for type checking to check if the first argument in Agent() is a Model instance or not. If not give a warning and handle gracefully, but note that it's deprecated. Then in Mesa 3.1 change the warning to an error, in 3.2 remove the code path.

EwoutH avatar Aug 19 '24 11:08 EwoutH

@GittyHarsha do you want to give it a shot with your current proposal? If so, feel free to open a draft PR!

Sure! I'm grateful for the oppurtunity

GittyHarsha avatar Aug 19 '24 11:08 GittyHarsha

I was going for type checking to check if the first argument in Agent() is a Model instance or not. If not give a warning and handle gracefully, but note that it's deprecated. Then in Mesa 3.1 change the warning to an error, in 3.2 remove the code path.

Why create such a long lasting impact on MESA 3? In my perception, with MESA 3 we can start with a clean sheat and brake stuff to make it better.

quaquel avatar Aug 19 '24 12:08 quaquel

You know by now I like to break things if that helps move us forward. I've broken enough to prove that (ask @rht).

But to break literately every mesa model built ever in existence is even for me a bit fanatic. If we can circumvent that with a few lines of code and a properly written warning, I think we should do that.

So if possible, there should be at least one version where the old one and the new both work, and where a warning is given how to move to the new one. The next one 3.0 should be that one. Because we don't support it in 3.0, we tolerate it with a warning to don't expect that we tolerate it much longer.

EwoutH avatar Aug 19 '24 13:08 EwoutH

@EwoutH I have opened a draft PR

Follow-ups

  1. Should I make next_item as a property of the model?
  2. should I add a warning when a user doesn't use automatic unique_id?
  3. Could you give pointers for adding tests and documentation for the changes introduced?

GittyHarsha avatar Aug 20 '24 03:08 GittyHarsha

So if possible, there should be at least one version where the old one and the new both work, and where a warning is given how to move to the new one.

Which was why I was suggeting another MESA 2 release with this (and perhaps other?) warnings in preperation for MESA 3.

quaquel avatar Aug 20 '24 07:08 quaquel

Implemented in #2260.

EwoutH avatar Sep 04 '24 13:09 EwoutH