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

Added homophily ratio in basic schelling example #2520

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vbv-shm
Copy link

@vbv-shm vbv-shm commented Nov 26, 2024

Changed homophily comparing logic as per discussion in #2515.
Now homophily_ratio is checked for agents to be happy. homophily_ratio is similar neighboring agents divided by the total number of neighbors.

@vbv-shm vbv-shm changed the title Fix #2515 as per discussions in issues. Added homophily ratio in in basic schelling example #2515 Nov 27, 2024
@vbv-shm vbv-shm changed the title Added homophily ratio in in basic schelling example #2515 Added homophily ratio in basic schelling example Nov 27, 2024
@@ -13,7 +13,7 @@ def __init__(
width: int = 40,
density: float = 0.8,
minority_pc: float = 0.5,
homophily: int = 3,
homophily_ratio: float = 0.3,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 0.3? It would make more sense to go to 3/8 (8 is the neighborhood size of a Moore grid with a radius of 1 and all fields occupied.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing. I will change 0.3 to 3/8 (0.375).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quaquel The step size of the slider is 0.1. So the ratio can either be 0.3 or 0.4.
If the ratio is 0.3, agents with 3 similar neighbors and 8 total neighbors will be happy, as 3/8 = 0.375.
And if the ratio is 0.4, agents with 3 similar neighbors and 8 total neighbors won't be happy.
So should I change 0.3 to 0.4?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can change the step size to 0.125 as well?

@EwoutH EwoutH added the example Changes the examples or adds to them. label Nov 27, 2024
@EwoutH
Copy link
Member

EwoutH commented Nov 27, 2024

Thanks for the PR! Could you update your PR description to contain all the necessary information to understand this change on its own? You can use this template if you want.

Edit: I saw there's also this PR open, which also modifies schelling:

Are there any conflicts between them? I would tend to let #2518 go first, since it was opened earlier. (or did we assign someone to this specific issue?)

@vbv-shm vbv-shm closed this Nov 27, 2024
@vbv-shm vbv-shm reopened this Nov 27, 2024
@vbv-shm
Copy link
Author

vbv-shm commented Nov 27, 2024

@EwoutH Thank you for the reply.
I have understood that #2815 will be given priority because it was opened first. Still, can I work on this PR just in case #2815 doesn't work out? Or is this PR required to be closed?
From now on, I will be careful not to open PR for some issue if PR is already opened.

@quaquel
Copy link
Member

quaquel commented Nov 27, 2024

I have provided feedback in #2518, if that is not addressed in the coming days and there is no further response, I am fine with merging this instead.

@vbv-shm
Copy link
Author

vbv-shm commented Nov 28, 2024

@quaquel Thank you. I understand.

@EwoutH EwoutH added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Nov 29, 2024
@vbv-shm
Copy link
Author

vbv-shm commented Dec 6, 2024

@EwoutH May you please run the benchmark tests?

@vbv-shm
Copy link
Author

vbv-shm commented Dec 9, 2024

@quaquel @EwoutH Previously the benchmark tests were failing because I changed homophily to homophily_ratio in the Schelling example. homophily was provided by the configuration file while the model now takes homophily_ratio as input. In order to fix this, I changed homophily to homophily_ratio inside the configuration.
I am curious if my fix is right or not. Could you please run the benchmark tests so that changes can be checked?

@EwoutH EwoutH added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -2.4% [-3.0%, -1.7%] 🔵 -0.8% [-1.0%, -0.6%]
BoltzmannWealth large 🔵 -1.3% [-2.5%, -0.1%] 🔵 +0.1% [-0.7%, +0.9%]
Schelling small 🔵 -1.0% [-1.4%, -0.7%] 🟢 -12.7% [-13.3%, -12.1%]
Schelling large 🔵 -0.2% [-0.7%, +0.4%] 🔴 +87.2% [+85.6%, +88.6%]
WolfSheep small 🔵 -2.6% [-3.3%, -1.9%] 🟢 -6.8% [-7.2%, -6.3%]
WolfSheep large 🟢 -4.8% [-5.5%, -3.9%] 🟢 -8.3% [-9.8%, -6.8%]
BoidFlockers small 🔵 -1.5% [-2.2%, -0.9%] 🔵 +2.3% [+1.5%, +3.1%]
BoidFlockers large 🔵 -2.8% [-3.9%, -1.6%] 🔵 +1.4% [+0.7%, +2.2%]

@vbv-shm
Copy link
Author

vbv-shm commented Dec 10, 2024

Looks like benchmarks ran, but performance is not up to the mark. So changing homophily to homophily_ratio inside the configuration worked.

@EwoutH
Copy link
Member

EwoutH commented Dec 10, 2024

Thanks for your work. We’re currently waiting on #2518 to be performance validated, one that’s done we can continue with rebasing this PR and checking it’s performance properly.

It’s great it now runs without problem!

@vbv-shm
Copy link
Author

vbv-shm commented Dec 11, 2024

Thank you

@quaquel
Copy link
Member

quaquel commented Dec 12, 2024

I have merged #2518, so let us know if you want to rebase this and further refine the example.

@vbv-shm
Copy link
Author

vbv-shm commented Dec 13, 2024

@quaquel Yes, I would like to refine this. A few things that I think can be updated are:

  1. Update configuration.py file for correct values for running benchmarks
  2. Change default value of homophily from 0.3
  3. Update slider step size in visualization
    As I add these changes, if something else is encountered, I will add that as well.
    Please provide your thoughts on this and anything else required to be added.

@EwoutH
Copy link
Member

EwoutH commented Dec 13, 2024

Great, make sure to git rebase first and resolve merge conflicts.

@vbv-shm vbv-shm force-pushed the main branch 3 times, most recently from aedb32f to 725c880 Compare December 14, 2024 13:09
@vbv-shm
Copy link
Author

vbv-shm commented Dec 14, 2024

@quaquelI @EwoutH I was running scheduling example locally, which is currently committed at the main branch (not my pull request).
image
No agents were getting happy, and the happy line is constant, as can be seen in the picture above.

But after I pressed the reset button, the grid shape changed, and it behaved normally, and agents are getting happy according to condition. Please see the below image:
image

I remember 2 weeks back when I ran it; it was running fine even without pressing reset, although the grid shape changed back then also.

@vbv-shm vbv-shm force-pushed the main branch 2 times, most recently from 77b20e4 to 4132e43 Compare December 14, 2024 18:11
@vbv-shm
Copy link
Author

vbv-shm commented Dec 16, 2024

@quaquel I have added changes in my latest update, which I feel are correct. Please only see the latest update and ignore the rest before it. The changes also fix the problem that I mentioned above.

@EwoutH EwoutH added trigger-benchmarks Special label that triggers the benchmarking CI and removed trigger-benchmarks Special label that triggers the benchmarking CI labels Dec 17, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 -0.6% [-1.4%, +0.1%] 🔵 -0.5% [-0.7%, -0.4%]
BoltzmannWealth large 🔵 +0.4% [-0.1%, +1.0%] 🔵 -0.5% [-0.9%, -0.0%]
Schelling small 🔵 +0.0% [-0.3%, +0.4%] 🟢 -50.9% [-51.1%, -50.8%]
Schelling large 🔵 +1.1% [+0.6%, +1.6%] 🟢 -44.2% [-44.9%, -43.3%]
WolfSheep small 🔵 +0.7% [+0.4%, +1.1%] 🔵 +0.3% [-0.7%, +1.4%]
WolfSheep large 🔵 +1.1% [+0.3%, +2.0%] 🔵 +1.5% [-0.2%, +3.0%]
BoidFlockers small 🔵 +1.2% [+0.6%, +1.8%] 🔵 +0.3% [-0.3%, +1.0%]
BoidFlockers large 🔵 +0.8% [+0.1%, +1.4%] 🔵 +0.5% [-0.1%, +1.0%]

@@ -47,7 +47,7 @@
"parameters": {
"height": 100,
"width": 100,
"homophily": 8,
"homophily": 0.4,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was 8 (the max for moore neighborhood, so it should be 1 now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quaquel I will change it.

Copy link
Member

@quaquel quaquel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one minor point remaining

@vbv-shm
Copy link
Author

vbv-shm commented Dec 19, 2024

@quaquel I am assuming you are referring to the step size of the slider. Should I change slider step size from 0.1 to 0.125? Then it also makes sense to change the required homophily ratio from 0.4 to 0.375.
If these changes are made or not, 4 out of 8 similar agents are required in neighbour in order for agent to be happy. That is why I was doubtful whether to add these.

@vbv-shm
Copy link
Author

vbv-shm commented Dec 20, 2024

@quaquel I have made the changes. Please check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Changes the examples or adds to them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants