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

examples: Clean-up Epstein Civil Violence and PD grid #2373

Closed
wants to merge 13 commits into from

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Oct 16, 2024

Clean up the epstein civil voilence models.

One issue is that a deepcopy currently isn't possible, and Solara currently uses that.

model1 = EpsteinCivilViolence()
import copy
# make deepcopy
model2 = copy.deepcopy(model1)

@quaquel would you like to pick this further up?

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -16.1% [-17.5%, -14.6%] 🔵 +0.2% [+0.0%, +0.3%]
BoltzmannWealth large 🔵 +2.1% [+1.6%, +2.6%] 🔵 +1.4% [+0.7%, +2.1%]
Schelling small 🔵 -1.8% [-2.4%, -1.2%] 🔵 -0.6% [-0.8%, -0.4%]
Schelling large 🔵 -0.9% [-2.4%, +0.5%] 🔵 +0.0% [-1.9%, +2.5%]
WolfSheep small 🔵 -0.7% [-1.0%, -0.3%] 🔵 +1.1% [+0.8%, +1.3%]
WolfSheep large 🔵 +1.4% [-0.2%, +2.9%] 🔴 +7.1% [+4.8%, +9.6%]
BoidFlockers small 🔵 -3.2% [-3.5%, -2.9%] 🔵 +0.6% [-0.2%, +1.3%]
BoidFlockers large 🔵 -0.5% [-1.4%, +0.2%] 🔵 +1.2% [+0.6%, +1.8%]

@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

This is a properly obscure problem. It arises from trying to deepcopy the OrthogonalMooreSpace. Even worse, whether or not the error triggers depends on the size of the grid (which suggests to me that perhaps recurssion error is triggered to early?). See also here

To test this yourself, try to deepcopy a moore / von neumann grid with varying sizes and see when it errrors out.

My hunch is that the problem starts from Cell.connections. If I comment out self.connections[key] = other in Cell.connect the problem disappears, supporting my hunch. One solution is to implement __setstate__ and __getstate__ explicitly for both Cell and Grid. Basically, don't return Cell.connections, and in Grid.__setstate__ after having initialized everything, rebuild the connections explicitly.

This is a major bug because it means that large experimental grids are currently not pickleable....

@Corvince any thoughts?

@quaquel quaquel added the bug Release notes label label Oct 16, 2024
@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

I have committed a possible fix in line with my previous comment. In short, In Cell.__getstate__, I set "connections" to an empty dict. In DiscreteSpace.__setstate__, I rebuild the connections via DiscreteSpace._connect_cells(). This latter method already existed in Grid and Voronoi, but not yet in network. I have thus moved the method stub to DiscreteSpace and implemented it for Network.

It needs further testing and additional unit tests for all DiscreteSpaces and large sizes, but it seems to work.

Also, this is a fix for Grids, not sure if the problem is present in all Discrete spaces....

@EwoutH
Copy link
Member Author

EwoutH commented Oct 16, 2024

Thanks! I will continue updating the examples, if this works for all of them than we can include this in a separate PR.

@quaquel
Copy link
Member

quaquel commented Oct 16, 2024

Thanks! I will continue updating the examples, if this works for all of them than we can include this in a separate PR.

Ok, I'll move the fix into a separate PR and add additional unit tests. Let me know how you get on with the other examples.

@EwoutH
Copy link
Member Author

EwoutH commented Oct 16, 2024

Awesome!

Updated pd_grid (Demographic Prisoner's Dilemma on a Grid). One issue is that we can't visualize any of the cell spaces currently. Without that the visualisations are quite useless. Tracking issue here:

@quaquel
Copy link
Member

quaquel commented Oct 18, 2024

I guess it would make sense to merge this even if #2377 is not addressed yet. It means the docs can progress and would give us something to use when developing #2377.

@EwoutH EwoutH marked this pull request as ready for review October 18, 2024 14:24
@EwoutH EwoutH changed the title examples: Clean-up Epstein Civil Violence examples: Clean-up Epstein Civil Violence and PD grid Oct 18, 2024
@EwoutH EwoutH added example Changes the examples or adds to them. and removed bug Release notes label labels Oct 18, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🟢 -8.1% [-9.4%, -6.6%] 🔵 -0.9% [-1.1%, -0.6%]
BoltzmannWealth large 🔵 +0.2% [-0.1%, +0.4%] 🔵 -3.6% [-4.8%, -2.4%]
Schelling small 🔵 -0.8% [-1.1%, -0.4%] 🔵 +0.1% [-0.1%, +0.3%]
Schelling large 🔵 +0.4% [-0.4%, +1.3%] 🔵 +1.1% [-1.5%, +4.1%]
WolfSheep small 🔵 +0.7% [+0.5%, +0.9%] 🔵 -0.2% [-0.4%, -0.0%]
WolfSheep large 🔵 +0.1% [-0.9%, +0.9%] 🔵 -1.6% [-3.3%, +0.2%]
BoidFlockers small 🔵 -2.1% [-2.8%, -1.4%] 🔵 -0.2% [-0.8%, +0.5%]
BoidFlockers large 🔵 -1.1% [-1.6%, -0.6%] 🔵 -0.8% [-1.2%, -0.2%]

@quaquel
Copy link
Member

quaquel commented Oct 18, 2024

What's there is, is good. Would it make sense to remove the notebooks?

@EwoutH
Copy link
Member Author

EwoutH commented Oct 21, 2024

This merge conflict, fuck.... couldn't we have merged this before the examples move?

@quaquel
Copy link
Member

quaquel commented Oct 21, 2024

Yep the merge conflict is massive (as with the docs stuff). Might be better to start a new branch and copy these changes into it (As you are doing with docs atm).

@quaquel
Copy link
Member

quaquel commented Oct 23, 2024

Do you mind if I pick this up as a new PR given the rebase/merge mess? I have time this afternoon (Wednesday 23 October) for simple cleanup work (but not for figuring out complicated stuff like the solara run import problem).

@quaquel
Copy link
Member

quaquel commented Oct 24, 2024

Superseded by #2408

@quaquel quaquel closed this Oct 24, 2024
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.

2 participants