Evaluate empties set more lazily

[Tortar]

This implementation is better than the one in #1543 , it builds the empties set only if it is required to do so with no breaking change.

[codecov]

Codecov Report

Base: 81.44% // Head: 81.42% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (906bc6d) compared to base (9bc7b1a).
Patch coverage: 76.47% of modified lines in pull request are covered.

❗ Current head 906bc6d differs from pull request most recent head 210799c. Consider uploading reports for the commit 210799c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1546      +/-   ##
==========================================
- Coverage   81.44%   81.42%   -0.03%     
==========================================
  Files          16       16              
  Lines        1326     1335       +9     
  Branches      230      233       +3     
==========================================
+ Hits         1080     1087       +7     
- Misses        203      204       +1     
- Partials       43       44       +1     

Impacted Files	Coverage Δ
mesa/space.py	91.25% <76.47%> (-0.32%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Tortar]

Tests are failing due to the move of examples folder to another repo, is there a way to use them though they are in another repo?

[Tortar]

we need actually to create a flag instead of using _empties in itself to check if it was built, because otherwise exists_empties could become very very slow when _empties was built but became empty Il Mar 6 Dic 2022, 10:13 rht ***@***.***> ha scritto:

…

***@***.**** commented on this pull request. ------------------------------ In mesa/space.py <#1546 (comment)>: > @@ -488,7 +504,7 @@ def move_to_empty( if self.is_cell_empty(new_pos): break else: - new_pos = agent.random.choice(sorted(self.empties)) + new_pos = agent.random.choice(sorted(self._empties)) self.empties is for public consumption. The inner workings might change, so using self.empties is more robust. Again, if the performance benefit is marginal, it's not worth it. — Reply to this email directly, view it on GitHub <#1546 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQH6VX4WZKYAV7UGCEIRZLDWL37UPANCNFSM6AAAAAASUY5YFM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Tortar]

ok, everything should be hopefully done

[rht]

self.empties_built doesn't add any perf benefit. Checking if self._empties is not an empty set already gives the same behavior.

[Tortar]

no it's not true, say the user built self.empties in some way, and then populate the list till its maximum capacity, so that self.empties becomes empty, then say the user checks (say at every step) that the maximum capacity has been reached calling exists_empty_cells then build.empties would be executed again and again because the set will remain empty each time even after building.

[Tortar]

wait but at the same time, it solves anyway the problem that can happen if the user instead of using self.exists_empty_cells uses len(self.empties) > 0, which can happen. Or more generally, whenever a read operation is done on self.empties for some reason if it is empty then you have this problem. Maybe it should be documented that using a flag has this benefit.

[Tortar]

Also using self._empties and not self.empties in exists_empty_cells would result in a bug if exists_empty_cells was called before the creation of self._empties through self.empties, because it would be an empty set

edit: I wrongly removed a comment where I agreed with you before these considerations so the structure of my comments make less sense now, but anyway I now think using the flag is much better in terms of user experience with the library

[rht]

I see, then it sounds good to me. Need to be documented, though. One option is to initialize self._empties = None and check for is not None, but this is less explicit.

[Tortar]

Added an explaination, feel free to modify/extend it if you think it can be improved :-)

[rht]

I saw from my inbox that you measured a 25% speedup in grid construction. Can't find it on GH. Did you remove it?

[Tortar]

yes, because I realized that it's a 1 time operation per model in the end (but anyway it was not a 25% speedup but a 4 times speed up for a 1000 x 1000 grid). I think the real perf benefits are 1) Less memory usage if empties is not built and 2) Something like (just guessing) ~15-25% speedup for place_agent, remove_agent, move_agent for Grid, SingleGrid and MultiGrid if empties is not built.

[rht]

These are significant improvements, and need to be communicated properly to the users. I think we should measure just one of the examples to see how much holistic speedup we get to running a simulation (without GUI), for a concrete illustration.

[Tortar]

After

from boltzmann_wealth_model.model import BoltzmannWealthModel

I run on a jupyter notebook:

%%prun
model = BoltzmannWealthModel()
for x in range(50):
    model.step()

Results:

• With github main -> from 0.115 to 0.125
• With this branch -> from 0.095 to 0.105

[rht]

What about sugarscape_cg? See if it is also ~10%.

[Tortar]

No it's not, it seems much smaller. There are many more operations in this model so it makes sense. Anyway it's more like ~20% in BoltzmannWealthModel, but it's one of the best models to track the perf of this one because move_agent() is one of the main operations there.

[rht]

OK, so we can at least say that BoltzmannWealthModel is ~20% because move_agent accounts for a significant portion of the agent step. But maybe just saying

~15-25% speedup for place_agent, remove_agent, move_agent

is more precise, because this statement is more localized, and doesn't depend on the rest of the code.

[Tortar]

yes, I agree that saying ~15-25% speedup for place_agent, remove_agent, move_agent is more informative 👍

[rht]

Though we need actual hard numbers instead of speculation of the speedup amount.

[Tortar]

i made a little script to check all cases:

import timeit

mock_agent = """
class a:
    def __init__(self,pos):
        self.pos=pos
agent= a(None); pos_1=(1,1); pos_2=(0,0)"""

grid_setup = """from mesa.space import Grid; grid = Grid(2,2,True);\n""" + mock_agent
grid_setup_2 = """from mesa.space_2 import Grid; grid = Grid(2,2,True);\n""" + mock_agent
multigrid_setup = """from mesa.space import MultiGrid; grid = MultiGrid(2,2,True);\n""" + mock_agent
multigrid_setup_2 = """from mesa.space_2 import MultiGrid; grid = MultiGrid(2,2,True);\n""" + mock_agent
singlegrid_setup = """from mesa.space import SingleGrid; grid = SingleGrid(2,2,True);\n""" + mock_agent
singlegrid_setup_2 = """from mesa.space_2 import SingleGrid; grid = SingleGrid(2,2,True);\n""" + mock_agent

ggrid_stmt_place = """grid.place_agent(agent, pos_1)"""
ggrid_stmt_remove = """grid.place_agent(agent, pos_1); grid.remove_agent(agent)"""
ggrid_stmt_move = """grid.place_agent(agent, pos_1); grid.move_agent(agent, pos_2)"""


setups = {"grid":[grid_setup, grid_setup_2],
          "singlegrid": [singlegrid_setup, singlegrid_setup_2],
          "multigrid": [multigrid_setup, multigrid_setup_2]}

for key,val in setups.items():

    x, y = val
    a = sum(timeit.repeat(ggrid_stmt_place ,x, number=1, repeat=100000))
    b = sum(timeit.repeat(ggrid_stmt_remove ,x, number=1, repeat=100000))
    c = sum(timeit.repeat(ggrid_stmt_move ,x, number=1, repeat=100000))
    d = sum(timeit.repeat(ggrid_stmt_place ,y, number=1, repeat=100000))
    e = sum(timeit.repeat(ggrid_stmt_remove ,y, number=1, repeat=100000))
    f = sum(timeit.repeat(ggrid_stmt_move ,y, number=1, repeat=100000))

    print(key)
    print("place_agent: ", a, d, d/a)
    print("remove_agent: ", b-a, e-d, (e-d)/(b-a))
    print("move_agent: ", c-a, f-d, (f-d)/(c-a))
    print()

This gives

grid
place_agent:  0.025405800177395577 0.03181019980183919 1.2520841532140299
remove_agent:  0.02350319959441549 0.0287818999368028 1.2245949672163599
move_agent:  0.06334279991096992 0.07534320030390518 1.1894516884287112

singlegrid
place_agent:  0.06369329994959116 0.07199410038083442 1.1303245465035219
remove_agent:  0.020794899848624482 0.02652919951651711 1.2757550990692526
move_agent:  0.097030600079961 0.10973119963455247 1.1308927239873312

multigrid
place_agent:  0.03021919996899669 0.036854399908406776 1.2195690139453543
remove_agent:  0.01850530034789699 0.04432980008641607 2.395519081183347
move_agent:  0.0661083995419176 0.09738600018317811 1.4731259697404744

So we arrive at 2.5x in multigrid case for removal! (Because the checking condition is simpler and the list cell has only one item inside). Another thing to notice is that place_agent in singlegrid is impacted less than the one in grid because of the super() call, I'm for the removal of it in another PR, I don't think it's more mantainable in this simple case.

[Corvince]

I didn't follow the complete conversation and hope there is nothing blocking, but I think the general approach seems quite clever @Tortar !

[Tortar]

@rht We have those hard numbers now on the speed-up👍

[rht]

OK, I'm merging. I will defer summarizing those results to later when writing up the release note.