-
Notifications
You must be signed in to change notification settings - Fork 927
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
Reimplementation of Continuous Space #2584
Conversation
Performance benchmarks:
|
for more information, see https://pre-commit.ci
Thanks for working on this!
Absolutely awesome, I fully support this design direction. I will do an API / conceptual level review tomorrow. Let me know when you would like to have a code/implementation level review. |
|
Okay, first of all, this is excellent work. While I could probably fledge our the API, your implementations are simply superior because you have considered every angle and detail. Especially the array stuff, I'm impressed. Let me review the API, comparing how to do stuff in the old and new continuous spaces.
# Current Implementation
space = ContinuousSpace(
x_max=1.0,
y_max=1.0,
torus=True,
x_min=0.0,
y_min=0.0
)
class MyAgent(Agent):
def __init__(self, unique_id, model):
super().__init__(unique_id, model)
agent = MyAgent(1, model) # New Implementation
space = ContinuousSpace(
dimensions=[[0, 1], [0, 1]], # More flexible, supports n-dimensions
torus=True,
random=model.random
)
class MyAgent(ContinuousSpaceAgent):
def __init__(self, space, model):
super().__init__(space, model)
agent = MyAgent(space, model) The new dimensions parameter is way more elegant and extensible, and I like the syntax. I'm not sure having to pass the space to the agent constructor is ideal, it might be able to be derived from the model. Also, an Agent might want to be part of multiple spaces (albeit on the same coordinate/position). Maybe the model can track which spaces there are. If there's one that could be the default, I will loop back to this later. Also for random, can we use
# Current Implementation
space.place_agent(agent, pos=(0.5, 0.5))
space.move_agent(agent, pos=(0.6, 0.6))
current_pos = agent.pos # Returns tuple (x,y) # New Implementation
agent.position = [0.5, 0.5] # Direct assignment
agent.position += [0.1, 0.1] # Vector arithmetic
current_pos = agent.position # Returns numpy array The new vector-based approach is much more intuitive and powerful, especially for physics-based simulations. The ability to use numpy array operations directly is a big win. I like the API and syntax, good stuff. The square brackets make it also feel more like an position/coordinate (probably very personal).
# Current Implementation
neighbors = space.get_neighbors(
pos=(0.5, 0.5),
radius=0.2,
include_center=True
)
# No built-in nearest neighbors functionality # New Implementation
# From agent's perspective
neighbors = agent.get_neigbors_in_radius(radius=0.2)
nearest = agent.get_nearest_neighbors(k=5)
# From space's perspective
distances = space.calculate_distances([0.5, 0.5]) Great to have this build in. The agent-centric approach is really elegant here. I would go with a single This also might be a place where we want to input the space or list of spaces (optionally). That way an Agent can search in a particular space. If there's only one space, we can default to that (and/or to the first space added to the model).
# Current Implementation
# Need to rebuild cache first
space._build_agent_cache()
all_positions = space._agent_points # numpy array
agents = list(space._agent_to_index.keys()) # New Implementation
all_positions = space.agent_positions # Direct access to numpy array
agents = space.agents # Returns AgentSet The new implementation is much cleaner and more efficient. No need to manually rebuild cache, and proper encapsulation of internal details. The AgentSet integration is a good choice. The direct access to positions through a property is more intuitive. Great stuff.
# Current Implementation
space.remove_agent(agent)
agent.pos = None # Must manually clear position # New Implementation
agent.remove() # Handles everything automatically The new implementation is clearly superior. This will save us a lot of bug reports in the long term.
# Current Implementation
is_valid = not space.out_of_bounds(pos=(0.5, 1.2))
adjusted_pos = space.torus_adj((1.2, 1.3)) # New Implementation
is_valid = space.in_bounds([0.5, 1.2])
adjusted_pos = space.torus_correct([1.2, 1.3]) I haven't used this feature often, but the new method names feel more intuitive ( Really amazing! |
Thanks! Some quick reactions
This is indeed one of the places were I struggled to cleanly seperate everything. However, I don't see any other way of doing it. I prefer explicit over implicit. Thus, I want to avoid having to make guesses about the attribute name of the space. I am skeptical about agents being in multiple spaces at the same time. Regardless, if a user want this, this is still possible by subclassing
Again, I prefer explicit over implicit. Moreover, this is identical to how it is handled in discrete spaces so it keeps the API consistent. Note that neither DiscreteSpace nor ContinuousSpace have
I am open to this suggestion, but not convinced yet. In most libraries I looked at last week, they cleanly seperate both cases because implementation wise they are quite different. For example, ball tree in sklearn has One option might be to add this as a third method?
The method changes the values that you pass, while this name suggests a boolean as a return. |
Practical use case: I have some layer of fixed agents (trees, intersections, houses) in a cell space, but I want other agents (birds, cars, people) to move between those in continuous space. Then I would like to have a Cell Space and a ContiniousSpace in the same model.
I would say random by default, with an argument to switch it to nearest. "Get 5 random agents with 100 meters of me" sounds like a common use case for me. The beautiful thing about a good single-method implementation is that you are very flexible with combinations of keyword arguments (again like we do with
And then when we think of a think of another selection criterea the number of functions could double again, of they all need an additional keyword argument. While I see your side of the argument, personally, I really like having a clean function name (
Internally it can be different code paths. |
Regarding a single However, while playing with this, I thought it might be useful to return not just the agents but also the distances. This makes it possible to do follow-up operations yourself easily: some_list = agent.get_neigbors_in_radius(radius=3.1415)
# sort by distance assuming some_list is [(agent, distance), (agent, distance)]
some_list.sort(key=lambda element:element[1])
# get k nearest agents within radius
k_agents = [entry[0] for entry in some_list[0:k]] |
I am sorry but I don't see the use case. You can just as easily have all agents with a permanent location inside the same continuous space. There is no need to use a cell space for this. Moreover, it also raises again the spectre of coordinate systems and their alignment across multiple spaces. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
While this is probably a move in the right direction anyway, and could be worth exploring on its own, it might be worth fully fledging our conceptual view on spaces first. Just to prevent doing double work. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@projectmesa/maintainers to review:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
A few initial comments and questions on the examples. Main code will be next.
@@ -6,10 +6,10 @@ | |||
|
|||
import numpy as np | |||
|
|||
from mesa import Agent | |||
from mesa.experimental.continuous_space import ContinuousSpaceAgent |
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.
What's you long term vision on the different Agent types in Mesa? Will each space need their own Agent 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.
I don't see a viable alternative at the moment other than having a set of different agent subclasses designed to work with different spaces.
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.
If I am understanding correctly, related #2585 (which is an awesome discussion BTW) then if a user has say someone in continouspace and part of network, then their agent would use inheritance to get those particular functions?
vision, | ||
separation, | ||
space, | ||
position=(0, 0), |
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.
Wasn't this supposed to be a sequence/array with (like [0, 0]
)?
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.
position in newstyle is np.typing.ArrayLike, so anything castable to np.array works.
The cleaner solution is to make position a positional argument rather than a keyword argument because this avoids the need for a default value. The nice thing of a kwarg, however, is that create_agents
becomes a bit more readable.
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.
A few initial comments/questions on the spaces code.
self._agent_positions: np.array = np.empty( | ||
(n_agents, self.dimensions.shape[0]), dtype=float | ||
) | ||
# self._agents: np.array = np.zeros((n_agents,), dtype=object) | ||
|
||
self.active_agents = [] | ||
self.agent_positions: ( | ||
np.array | ||
) # a view on _agent_positions containing all active positions | ||
|
||
self._n_agents = 0 | ||
|
||
self._index_to_agent: dict[int, Agent] = {} | ||
self._agent_to_index: dict[Agent, int | 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.
Seems like 2 variables and 4 private variables to track agents and their positions. Could you add some docstring about this, for future reference?
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 docstring is the right place for that. I can add some more comments, however.
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.
Maybe we needs some dev docs or something. PR description could also work (for now).
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 added some comments, let me know if this is sufficient.
Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
for more information, see https://pre-commit.ci
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.
@quaquel Awesome as always! I always enjoy learning from your code. I had a few questions, but fantastic!
@@ -6,10 +6,10 @@ | |||
|
|||
import numpy as np | |||
|
|||
from mesa import Agent | |||
from mesa.experimental.continuous_space import ContinuousSpaceAgent |
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.
If I am understanding correctly, related #2585 (which is an awesome discussion BTW) then if a user has say someone in continouspace and part of network, then their agent would use inheritance to get those particular functions?
@@ -58,47 +61,31 @@ def __init__( | |||
|
|||
def step(self): | |||
"""Get the Boid's neighbors, compute the new vector, and move accordingly.""" | |||
neighbors = self.model.space.get_neighbors(self.pos, self.vision, True) | |||
neighbors, distances = self.get_neighbors_in_radius(radius=self.vision) |
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.
This is very clean!
|
||
import numpy as np | ||
from numpy.typing import ArrayLike | ||
from scipy.spatial.distance import cdist |
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 am conflicted about adding a core dependency for one call, although it is scipy. Is there thoughts or plan to grow this to add more types of calculations (e.g. non Euclidian) or use scpiy in other spots?
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.
Its part of anaconda, the code is roughly 4 times faster than a numpy only version, and you can use other distances with it, so I think it is worth it, but it can be changed if others see this differently.
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 it's not ideal, but scipy is a common one, We could include it in the recommended (but not core) dependencies, like we do with pandas.
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.
My bias is always for faster so I say leave it in as core or add to recommended (scipy is a part of examples). (I am also tracking numpy, pandas, and tqdm as core
I also proposed adding **kwargs
to calculate_distances
below so user can exploit some of the other cdist capability
|
||
return delta | ||
|
||
def calculate_distances( |
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.
For for these three functions 1- calculate_distances
, 2- get_agents_in_radius
, 3-get_k_nearest_neighbors
it seems the return structure changes - 1- array, list, 2- (list, array), 3-list, array
This seems to violate the principle of least surprise --- is there a reason for this I am just not understanding?
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.
That is fair point; I have been wondering about that as well.
The main difference is that calculate_distances
focuses on distances, so I have this as the first return. The other 4 (ContinousSpace.get_agents_in_radius
, ContinousSpace.get_k_nearest_agents
, and their ContinuousSpaceAgent
versions ) all are about getting agents, so I return those first.
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.
My bias would be to keep them all array, list this may be because I have been burned with GIS libraries switching long, lat to Lat, long too many times. However, I will leave it up to you
To optimize the use of the scipy library would you want to add **kwargs
so users could change some of the key word arguments like metric
?
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 added the kwargs idea, but this only works if torus is not true.
You can probably just do that with multiple inheritance at the moment, we might want to flesh that out further in the near future. |
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.
Let's get this in, and start building with it.
Thanks for driving this home Jan!
This reimplements continuous space and adds the basic starting point for an agent-centric API analogous to what is possible with cell spaces.
API examples
This reimplements
ContinuousSpace
in line with the API design established for cell spaces. A key design choice of the cell spaces is that movements become agent-centric (i.e.,agent.cell = some_cell
). This PR does the same but for continuous spaces. So, you can doagent.position += speed * heading
. Likewise, you can doagent.get_nearest_neighbors(k=5)
oragent.get_neigbors_in_radius(radius=3.1415)
.Implementation details
In passing, this PR contains various performance improvements and generalizes continuous spaces to n-d.
agent.position
is a view into the numpy array with all agent positions. This is analogous to how cells access their value in a property layer. In contrast to the current implementation, the numpy array with all agent positions is never fully rebuilt. Rather, it is a masked array with possible empty rows. If the numpy array becomes too small, it is expanded by adding 20% more rows to it. Most of this will be fine-tuned further, becoming user controllable.Regarding performance, I have spent most of last week reading up on r-trees, ball trees, and kd trees. Moreover, I ran various performance tests comparing trees against brute force distance calculations. It seems that brute force wins for MESA's use case. Why? Trees are amazing if you need to do frequent lookups and have comparatively few updates of locations. In MESA, however, we will often have many updates of locations (any agent movement will trigger an update of the location and, thus, an update of the tree). Once I established that brute force was the way to go, Next, I compared various ways of calculating Euclidian distances. I settled on using
scipy.spatial.cdist
for non-torus and and afor
loop over the columns for manual Euclidian distance calculations in case of torus.