-
Notifications
You must be signed in to change notification settings - Fork 892
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
Fix: Schelling Model Neighbor Similarity Calculation #2518
base: main
Are you sure you want to change the base?
Fix: Schelling Model Neighbor Similarity Calculation #2518
Conversation
Performance benchmarks:
|
else: | ||
self.model.happy += 1 | ||
# If unhappy, move to a random empty cell | ||
if similarity_fraction < self.model.homophily / 8.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.
why not change homopily at the model level to be a fraction? That makes the model independent of the neighborhood size.
if hasattr(neighbor, "type") and neighbor.type == self.type | ||
] | ||
total_neighbors = [ | ||
neighbor for neighbor in neighbors if hasattr(neighbor, "type") |
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.
you iterate twice, with one check being done in both. You can make this more efficient by looping only once.
@Sahil-Chhoker Thanks for your PR. When do you expect to be able to incorporate @quaquel's feedback? If you have any question about it feel free to ask! |
Thank you, @quaquel, for your review! Could you please provide more details about the model and how can I can make changes to it? |
My feedback is quite clear, and there is a link to a more detailed description of the model in the original issue. I requested 2 well-defined changes to this PR: change From your reaction, I therefore deduce that you are not particularly familiar with agent-based modeling. |
similar_neighbors = 0 | ||
|
||
for neighbor in neighbors: | ||
if hasattr(neighbor, "type"): # Exclude empty cells |
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 not needed, iter_neighbors
returns a list of agents. In this model all agents have the type
attribute.
self.pos, moore=True, radius=self.model.radius | ||
) | ||
|
||
# Filter out empty cells | ||
similar_neighbors = [ | ||
neighbor | ||
for neighbor in neighbors | ||
if hasattr(neighbor, "type") and neighbor.type == self.type | ||
] | ||
total_neighbors = [ | ||
neighbor for neighbor in neighbors if hasattr(neighbor, "type") | ||
] | ||
|
||
# Calculate fraction of similar neighbors | ||
if len(total_neighbors) > 0: | ||
similarity_fraction = len(similar_neighbors) / len(total_neighbors) | ||
valid_neighbors = 0 | ||
similar_neighbors = 0 | ||
|
||
for neighbor in neighbors: | ||
if hasattr(neighbor, "type"): # Exclude empty cells | ||
valid_neighbors += 1 | ||
if neighbor.type == self.type: # Count similar neighbors | ||
similar_neighbors += 1 | ||
|
||
# Calculate the fraction of similar neighbors | ||
if valid_neighbors > 0: | ||
similarity_fraction = similar_neighbors / valid_neighbors | ||
|
||
# If unhappy, move to a random empty cell | ||
if similarity_fraction < self.model.homophily / 8.0: | ||
if similarity_fraction < self.model.homophily: | ||
self.model.grid.move_to_empty(self) | ||
else: | ||
self.model.happy += 1 |
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.
neighbors = self.model.grid.get_neighbors(
self.pos, moore=True, radius=self.model.radius
)
# Count similar neighbors
similar_neighbors = len([n for n in neighbors if n.type == self.type])
# Calculate the fraction of similar neighbors
if (valid_neighbors := len(neighbors) )> 0:
similarity_fraction = similar_neighbors / valid_neighbors
# If unhappy, move to a random empty cell
if similarity_fraction < self.model.homophily:
self.model.grid.move_to_empty(self)
else:
self.model.happy += 1
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.
see my second round of feedback
Please switch from
currently, as you can see the tests fail because the model won't run. The list expression exhausts the iterator, so getting the number of valid neighbors fails. |
Performance benchmarks:
|
self.pos, moore=True, radius=self.model.radius | ||
) | ||
|
||
# Count similar neighbors | ||
similar = sum(neighbor.type == self.type for neighbor in neighbors) | ||
similar_neighbors = len([n for n in neighbors if n.type == self.type]) |
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.
The model looks significantly slower. I’m curious if we could minimize that performance degradation, I suspect the main problem is with this line doing additional work.
(But not sure, haven’t profiled it)
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.
The slowdown is not due to this line of code but because the behavior changes a lot. Prior to this PR, the model would reach steady state in a few ticks. Now, it can take many ticks and frequently the model never reaches stable state.
Summary
Fixed #2515 Schelling segregation model to calculate agent happiness using neighbor similarity fraction, aligning with Wikipedia and NetLogo standards.
Bug / Issue
Current model incorrectly counts empty spaces as neighbors, leading to inaccurate agent happiness determination.
Implementation
Modified
SchellingAgent.step()
to: