simplify get neighborhood

[Corvince]

I had sworn to myself that I wouldn't touch the get_neighborhood function again, since I have already spent too much time with it. But I also wasn't happy with it, because I think it is has become rather unreadable (and thus unhackable). But it is one of few functions where I think it is worth the extra performance, since it is called potentially millions of time in an abm simulation.

However, recently I imagined a slightly different approach that would be simpler and faster. And it turns out it works! By checking first if we are well inside the grid we can skip the boundary checks and thus have a faster neighborhood calculation. Close to the edge I reimplemented a simpler, albeit slower algorithm compared with the status quo. But few cells are on the border and most are in the center. So in total we get a faster algorithm with simpler code.

Additionally I special-cased the radius=1 case, since it is so common. In that case we can skip calculations and just return the neighborhood directly.

[Corvince]

Tagging @Tortar since you spent so much time as well with the current implementation

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (1ea0dc7) 80.66% compared to head (d2908cc) 80.66%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1760   +/-   ##
=======================================
  Coverage   80.66%   80.66%           
=======================================
  Files          15       15           
  Lines         869      869           
  Branches      183      185    +2     
=======================================
  Hits          701      701           
  Misses        147      147           
  Partials       21       21           

Files Changed	Coverage Δ
mesa/space.py	90.88% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Tortar]

By how much this is faster than the previous one? I think that anyway the set look-up removes a lot of the extra benefit in doing this kind of optimizations since the caching is much faster anyway (but this was true also when changing the implementation before), but I think that you are well aware of this.

I also saw that you used a list transformation at the end of the function but isn't a sorted better for reproducibility?

[Corvince]

Yes you are right, it doesn't really matter a lot. That's why I would prefer a simple solution :)

The new strategy (without radius-1 shortcut) is around 10% faster. Not much, but more importantly not slower than before.

The radius-1 strategy is around twice as fast. But maybe it should still be removed to keep it simple, since it really only matters the first time a neighborhood is accessed.

You are right about the set - it would require calling sorted() but that would be slow. Better to (ab)use a dictionary to keep insertion order.

[rht]

I agree with removing the radius=1 code branch.

I also wonder if GPT-4 + Code Interpreter can iteratively improve the perf, while keeping it readable.

[Corvince]

Not that easily. I already tried code interpreter, but it needs quite some hand holding ("prompt engineering").

That is it starts out too vague/general. I then asked specifically about get neighborhood. That gave some more concrete advice, but I don't think it would have been faster. And it missed some corner cases.

Not disappointed by GPT4 here, I just think it is already pretty optimized. Might work for other functions, but I haven't asked specifically about them yet.

[rht]

Minmaxing the drawbacks and benefits, I am in favor of this PR. Even though:

1. the skipping of the boundary checks could have been incorporated into the current algorithm, with everything else intact.
2. the slower boundary checks version is essentially the previous version.

Also, it shouldn't matter much, since we will have another contender in the performance land in the form of nogil CPython (could be in 3.13, which is ~1 year from now), and that get_neighborhood is a self-contained function that can be easily ported to Cython for maximum performance improvement.

Let me know if this PR has been finalized.

[rht]

This PR seems to be closer to Agents.jl implementation: https://github.com/JuliaDynamics/Agents.jl/blob/7bc8ceac5bd569d8e526fb23a14d8c163c16f665/src/spaces/grid_general.jl#L131-L159.

[Corvince]

Minmaxing the drawbacks and benefits, I am in favor of this PR. Even though:

1. the skipping of the boundary checks could have been incorporated into the current algorithm, with everything else intact.
2. the slower boundary checks version is essentially the previous version.

Also, it shouldn't matter much, since we will have another contender in the performance land in the form of nogil CPython (could be in 3.13, which is ~1 year from now), and that get_neighborhood is a self-contained function that can be easily ported to Cython for maximum performance improvement.

Let me know if this PR has been finalized.

Yes with the current implementation the same could have been done for maximizing performance. But as I said this PR aims at simplification and thus increasing maintainability. That it is slightly faster is totally negligible.

And the PR should be ready for review now