nkremerh / sugarscape

Development repository for the Digital Terraria Lab implementation of the Sugarscape agent-based societal simulation.
https://github.com/digital-terraria-lab/sugarscape
MIT License
7 stars 12 forks source link

Replaces insertion sort and reverse with reverse order insertion sort #28

Closed colinhanrahan closed 8 months ago

colinhanrahan commented 8 months ago

Whenever sortCellsByWealth() is called, reverse() is called immediately after. Sorting the cells in reverse order in the first place is more efficient. This commit removes the sortCellsByWealth() function, replaces it with sortCellsByWealthDescending() (which sorts in descending order), and replaces two instances of calls to sortCellsByWealth() and then reverse() with sortCellsByWealthDescending().

goolabjamun commented 8 months ago

Perhaps we can consider using the built-in Python sort routine or some other sort that operates in O(n log n) time instead of O(n^2) time.

colinhanrahan commented 8 months ago

The lists passed in to this function are very small — In my tests, both insertion sort and quicksort take 0.0000 seconds. std::sort() in C++ actually prefers insertion sort for smaller lists because it's faster. @nkremerh I actually don't think sorting the cells is ever necessary in findBestEthicalCell (the only place the sort function is called). The Binary decision models iterate through the ethical values and find the first positive one anyway. (If there is a correlation between wealth and ethical value, removing the shuffle will change the behavior somewhat, but the decision is still naive and binary.) The Top models only need to find the best ethical value in the list, which can be achieved with a max rather than a sort.

nkremerh commented 8 months ago

It is not strictly necessary, that's correct. However, the sorting places the options in greedy order (emulating the intended behavior of the original Sugarscape) then finds the first ethically acceptable option from that list. The intended effect is that the ethical decision model is placed atop Sugarscape rather than replacing things whenever possible.

colinhanrahan commented 8 months ago

That makes sense. I was considering using a max for the "if Top" block instead of sorting, but it seems less readable than just calling the function again and its impact seems to be negligible. After testing, insertion sort seems appropriate for the small list size, and I renamed sortCellsByWealthDescending back to sortCellsByWealth in a commit on this branch. Should be OK to merge.

nkremerh commented 8 months ago

Good work!