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

bug in ContinuousSpace.get_neighbors #2592

Closed
quaquel opened this issue Jan 3, 2025 · 4 comments · Fixed by #2599
Closed

bug in ContinuousSpace.get_neighbors #2592

quaquel opened this issue Jan 3, 2025 · 4 comments · Fixed by #2599
Labels
bug Release notes label

Comments

@quaquel
Copy link
Member

quaquel commented Jan 3, 2025

Describe the bug
Currently, get_neighbors will fail to identify neighbors if two agents occupy the exact same position.

The line that causes the issue is shown below. if include_center is True, this will return all agents on the position. If if include_center is False, it will return no agents. However, in case of 2 agents on the same location, neither is correct.

        neighbors = [
            self._index_to_agent[x] for x in idxs if include_center or dists[x] > 0
        ]

Expected behavior
Return any neighbors that are not the agent for which you are calling get_neighbors.

To Reproduce
Create a space with 2 agents in the same location and call get_neighbors with the position of either agent.

model = Model(seed=42)
space = ContinuousSpace(1, 1, torus=False)
agent1 = Agent(model)
space.place_agent(agent1, (0.5, 0.5))

agent2 = Agent(model)
space.place_agent(agent2, (0.5, 0.5))

# returns no agents, should return agent2
space.get_neighbors(agent1.pos, radius=1, include_center=False)

# returns both agents, should return only agent2
space.get_neighbors(agent1.pos, radius=1, include_center=True)
@quaquel quaquel added the bug Release notes label label Jan 3, 2025
@Sahil-Chhoker
Copy link
Collaborator

Sahil-Chhoker commented Jan 4, 2025

Upon looking at code I found that it is impossible to find the agent which is passed in the method, because you are passing the position, and since every agent has the same position, if you try to remove the so called current agent, you will have to remove a random one or none at all.

To maybe overcome this one can change the type of the pos variable, that is if an agent is passed in the method, we can track that agent and exclude it from the result.

Can something like this work?

if isinstance(pos, Agent):
            self.current_agent = pos
            pos = pos.pos
        else:
            self.current_agent = None

...

neighbors = [
            self._index_to_agent[x] for x in idxs
        ]

if not self.current_agent:
    return neighbors
else:
    neighbors.remove(self.current_agent)
    return neighbors

@quaquel
Copy link
Member Author

quaquel commented Jan 4, 2025

You are absolutely right that this bug cannot be fixed inside the method as it currently stands. However, changing the method signature is a breaking change and could have a profound impact on existing models. So, that is not an ideal solution.

Practically speaking, with #2584 almost ready to go, I am thinking of the following

  1. explicitly document the current buggy behavior. This is the minimum thing to do.
  2. Add a new method with a different signature with the correct behavior. Not sure about this in light of Reimplementation of Continuous Space #2584.
  3. Update the boid flocking example to use Reimplementation of Continuous Space #2584, the new method, or work around the bug. The latter is, again, a minimum thing to do.

@EwoutH
Copy link
Member

EwoutH commented Jan 6, 2025

# returns no agents, should return agent2
space.get_neighbors(agent1.pos, radius=1, include_center

I don't think I agree this is unintended behavior. The argument is called include_center, not include_self. By setting it (the former) False, it does exactly what you expect: Return neighbors not at the same location as you.

That said, it might be helpful to have an include/exclude self argument. We could add that to the new API.

@quaquel
Copy link
Member Author

quaquel commented Jan 6, 2025

I don't think I agree this is unintended behavior.

Let's agree to disagree. The documentation just clarifies the behavior even more.

That said, it might be helpful to have an include/exclude self argument. We could add that to the new API.

The problem cannot occur there because I draw a sharp distinction between getting agents from a space and getting neighbors from an agent.

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

Successfully merging a pull request may close this issue.

3 participants