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

RandomActivation scheduler not working after upgrade 2.1 -> 2.2.3 #2006

Closed
profConradi opened this issue Jan 24, 2024 · 14 comments
Closed

RandomActivation scheduler not working after upgrade 2.1 -> 2.2.3 #2006

profConradi opened this issue Jan 24, 2024 · 14 comments
Assignees
Labels
bug Release notes label

Comments

@profConradi
Copy link

profConradi commented Jan 24, 2024

I upgraded 2.1 to 2.2.3 and in this simple test model RandomActivation scheduler does not work anymore: agent activation is done sequentially.
Furthermore if I not call super().init() in the model class, the first agent is not added to the scheduler. Is this a bug or a mistake on my part?

    class CognitiveAgent(mesa.Agent):
        """Agents make decisions using automatic (e.g., reflexive) versus controlled (e.g., deliberative) cognition,
        interact with each other, and influence the environment (i.e., game payoffs)."""
    
        def __init__(self, unique_id, model):
            # Pass the parameters to the parent class.
            super().__init__(unique_id, model)
    
            # Each agent is defined by a single parameter x that represents its probability of controlled processing
            self.xdx = np.random.uniform(0., 1.)
    
        def step(self):
            # The agent's step will go here.
            # For demonstration purposes we will print the agent's unique_id
            other_agent = self.random.choice(self.model.schedule.agents)
            print(f"I'm {self.unique_id} and I choose {other_agent.unique_id}")
            
    class CognitiveModel(mesa.Model):
        """A model with some number of agents."""
    
        def __init__(self, N):
            super().__init__()
            self.num_agents = N
            self.schedule = mesa.time.RandomActivation(self)
            # Create agents
            for i in range(self.num_agents):
                a = CognitiveAgent(i, self)
                # Add the agent to the scheduler
                print(a,i)
                self.schedule.add(a)
    
        def step(self):
            """Advance the model by one step."""
            # The model's step will go here for now this will call the step method of each agent and print the agent's unique_id
            self.schedule.step() 
@EwoutH
Copy link
Member

EwoutH commented Jan 24, 2024

Thanks for reaching out. I will test this tomorrow morning, if no one else has done it by then.

Furthermore if I not call super().init() in the model class, the first agent is not added to the scheduler.

Calling super().__init__() was always recommended, but now it’s required. It initializes Model.agents among other things. You’re using the Mesa Model class, so you should properly initialize it.

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

@rht do you have time to investigate this?

@rht
Copy link
Contributor

rht commented Jan 26, 2024

I'd need some help debugging all the problems caused by 2.1.x -> 2.2.x. Hands are full with elsewhere.

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

Unfortunately, I can reproduce this issue for both Mesa 2.2.0 and 2.2.3, while the correct behavior (shuffled agent steps) on Mesa 2.1.5. @projectmesa/maintainers this is a major issue and needs immediate care, we broke RandomActivation.

I will investigate further.

@EwoutH EwoutH transferred this issue from projectmesa/mesa-examples Jan 26, 2024
@EwoutH EwoutH self-assigned this Jan 26, 2024
@EwoutH EwoutH added the bug Release notes label label Jan 26, 2024
@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

I've narrowed it down to the fact that the AgentSet's shuffle(inplace=True) doesn't shuffle the agents. Could be because the shuffle isn't working, or the in place updating isn't.

mesa/mesa/time.py

Lines 144 to 147 in 99b35fc

def do_each(self, method, shuffle=False):
if shuffle:
self.agents.shuffle(inplace=True)
self.agents.do(method)

Screenshot_366

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

It's the inplace updating that isn't working. Without it, it works correctly:

Screenshot_367

@quaquel you wrote this code and know more about the AgentSet implementation. Could assist in fixing the inplace updating?

@EwoutH EwoutH pinned this issue Jan 26, 2024
@profConradi
Copy link
Author

profConradi commented Jan 26, 2024

Thanks for your work, Mesa is an amazing framework!

@quaquel
Copy link
Member

quaquel commented Jan 26, 2024

I can try this afternoon. I don't see an obvious mistake in the code.

AgentSet.shuffle

        shuffled_agents = list(self)
        self.random.shuffle(shuffled_agents)

        return (
            AgentSet(shuffled_agents, self.model)
            if not inplace
            else self._update(shuffled_agents)
        )

random.shuffle is inplace. List(self) returns a list of hard refs to the agents.

AgentSet._update

    def _update(self, agents: Iterable[Agent]):
        """Update the AgentSet with a new set of agents.
        This is a private method primarily used internally by other methods like select, shuffle, and sort.
        """

        self._agents = weakref.WeakKeyDictionary({agent: None for agent in agents})
        return self

We create a new WeakKeyDictionary so it should shuffle.

Accidentally, This is the code I was playing with for #1993. So my to do;

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

Thanks. I had already written an additional unittest for RandomActivation itself, we can test updates also against that (#2007).

@quaquel
Copy link
Member

quaquel commented Jan 26, 2024

Found it. The problem is in the scheduler not in AgentSet.

do_each should be

    def do_each(self, method, shuffle=False):
        if shuffle:
            self._agents.shuffle(inplace=True)
        self._agents.do(method)

quaquel added a commit to EwoutH/mesa that referenced this issue Jan 26, 2024
@quaquel
Copy link
Member

quaquel commented Jan 26, 2024

I committed the fix to #2007.

quaquel added a commit to EwoutH/mesa that referenced this issue Jan 26, 2024
quaquel added a commit to EwoutH/mesa that referenced this issue Jan 26, 2024
fix for the second issue in projectmesa#2006. If super is not present, we create the data structure but forget to add the agent to it. This is just a backward compatibility fix.
@rht rht closed this as completed in 29f5dad Jan 26, 2024
EwoutH added a commit that referenced this issue Jan 26, 2024
…2007)

* tests: Add test to check if RandomActivation is not sequential

Adds a test that checks if the RandomActivation doesn't trigger agents in a sequential order.

In theory this could give false positives (a test passing when it shouldn't, but that chance is around ~0.1^18).

* fix for RandomActivation bug

fixes #2006

* add agentset.shuffle unittest

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* ruff fix

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add agent to model.agent correctly even if super was not called

fix for the second issue in #2006. If super is not present, we create the data structure but forget to add the agent to it. This is just a backward compatibility fix.

* test: Shuffle more agents to prevent false negatives

No the chance on a false negative is one in 12! instead of 4! (40 million instead of 24)

---------

Co-authored-by: Jan Kwakkel <j.h.kwakkel@tudelft.nl>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
EwoutH added a commit that referenced this issue Jan 26, 2024
…2007)

* tests: Add test to check if RandomActivation is not sequential

Adds a test that checks if the RandomActivation doesn't trigger agents in a sequential order.

In theory this could give false positives (a test passing when it shouldn't, but that chance is around ~0.1^18).

* fix for RandomActivation bug

fixes #2006

* add agentset.shuffle unittest

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* ruff fix

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add agent to model.agent correctly even if super was not called

fix for the second issue in #2006. If super is not present, we create the data structure but forget to add the agent to it. This is just a backward compatibility fix.

* test: Shuffle more agents to prevent false negatives

No the chance on a false negative is one in 12! instead of 4! (40 million instead of 24)

---------

Co-authored-by: Jan Kwakkel <j.h.kwakkel@tudelft.nl>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

@profConradi we just released Mesa 2.2.4 which should fix this issue. Could you confirm this is the case?

@profConradi
Copy link
Author

Yes, the RandomActivation Scheduler of 2.2.4 works correctly! Thanks!

@EwoutH
Copy link
Member

EwoutH commented Jan 26, 2024

Great to hear, thanks a lot for reporting this issue!

For such a serious bug to slip thought the the review, test and release process is really unfortunate. I wrote some thoughts on it here: #1909 (comment)

@rht rht unpinned this issue Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release notes label
Projects
None yet
Development

No branches or pull requests

4 participants