Make legal_positions a static attribute instead of a dynamic property

[otizonaizit]

The bot attribute bot.legal_positions used to be a property, i.e. the list of positions got generated on the fly every time the code tried to access bot.legal_positions. This could be surprising, especially if the code was trying to implement something like "remove the enemy positions from the legal positions so that I run accidentally on top of an enemy". We have seen repeatedly code like this:

1 if bot.enemy[0].position in bot.legal_positions:
2     bot.legal_positions.pop(bot.enemy[0].position)
3 if bot.enemy[1].position in bot.legal_positions:
4     bot.legal_positions.pop(bot.enemy[1].position)
5 
6 for new_pos in bot.legal_positions:
    ...

The problem with this code if bot.legal_position is a dynamically generated list is that it would run without generating any error, but does not do what the coder thinks, because bot.legal_positions in line 6 will still be unaltered, i.e. it will still contain the enemy positions.

As there's no special reason to have it this way, this PR changes bot.legal_positions to be a static attribute, generated once at Bot's instantiation time. There are no backward compatibility concerns: if people were trying to write to bot.legal_positions before they had to make a copy anyway, so their code will continue to work. This PR is only making new things possible that weren't possible before.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (bb147a5) 85.74% compared to head (bca9f52) 85.72%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #786 +/- ## ========================================== - Coverage 85.74% 85.72% -0.02% ========================================== Files 21 21 Lines 2371 2368 -3 ========================================== - Hits 2033 2030 -3 Misses 338 338 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[otizonaizit]

by the way, the failed checks are unrelated (some remote tests that keep on failing intermittently)

[otizonaizit]

should we reconfigure coveralls not to complain if the decrease in coverage is below -1%? I can't figure why the coverage would increase for Pythons >= 3.11 but decrease instead for Python < 3.11?

[Debilski]

Coverage fell in all editions in team.py (fewer lines means less % coverage, which makes this kind of a stupid metric anyway). But in 3.11 and 3.12 an error is triggered at some point, which covers three additional lines with the except clause 🙃.

I’m all for making this less annoying.

[Debilski]

As for the proposed change in legal_positions: I agree that it makes things more consistent (homezone, position and others being simple attributes as well [*]).

However, one thought: I think that the described legal_positions.pop scenario has a flaw which makes it somewhat problematic. Once you have popped too many times (which may only happen once in 100 games), the legal_positions list will be empty. Obviously, you will need to have a list of fall back values to find some place to go to and you are back at writing original_legal_positions = bot.legal_positions.copy() again.

If our plan is to show the .pop approach in a tutorial, it should come with a disclaimer: ‘Feel free to change the legal_positions but be careful what you wish for ’. In my own bot I would not change the default variables and interpret them as immutable simply for stylistic reasons (functional programming ftw :) ).

[]: In fact homezone is (I think) a cached* variable, so changes would even be permanent between rounds.

[otizonaizit]

However, one thought: I think that the described legal_positions.pop scenario has a flaw which makes it somewhat problematic. Once you have popped too many times (which may only happen once in 100 games), the legal_positions list will be empty. Obviously, you will need to have a list of fall back values to find some place to go to and you are back at writing original_legal_positions = bot.legal_positions.copy() again.

yes, that is a possibility. I mean, legal_positions always contains bot.position too, so if you are popping that you are looking for trouble anyways. And we don't need to protect people from shooting themselves in the foot (Python consenting adults philosophy and so on...). But at least they'll get an Exception. Now they don't get any feedback on the fact that the code doesn't do what they think it should do. I think the trade off is pretty clearly on the side of making this thing a proper list.

If our plan is to show the .pop approach in a tutorial, it should come with a disclaimer: ‘Feel free to change the legal_positions but be careful what you wish for ’.

yes, if this gets merged I'll adapt the demo bot to use this and will put a comment on it to make it clear.

[Debilski]

This doubles the time of the StoppingPlayer benchmark in demo/benchmark_game.py, since we pre-calculate it for all four bots at each step (which means we iterate 4x5 times through self.walls every time as it is not hashed), although it is hardly ever needed at all for a noisy enemy. :/

Other than that we can merge. At least the resulting list of legal positions is cached now. :)

[otizonaizit]

Ah, but that's not a realistic benchmark. In all other demos we actually use legal_positions, so those are going to be faster, if anything at all...

26 Feb 2024 18:22:04 Rike-Benjamin Schuppner @.***>:

This doubles the time of the StoppingPlayer benchmark in demo/benchmark_game.py, since we pre-calculate it for all four bots at each step (which means we iterate 4x5 times through self.walls every time as it is not hashed), although it is hardly ever needed at all for a noisy enemy. :/

Other than that we can merge. At least the resulting list of legal positions is cached now. :)

— Reply to this email directly, view it on GitHub[https://github.com/ASPP/pelita/pull/786#issuecomment-1964697335], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AACUYC7UFQ2B337UXTT5573YVTADTAVCNFSM6AAAAABDGQ5JDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUGY4TOMZTGU]. You are receiving this because you authored the thread. [Tracking image][https://github.com/notifications/beacon/AACUYC4VOODNXKGDBQ5V5JDYVTADTA5CNFSM6AAAAABDGQ5JDSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVDLTPO.gif]

[Debilski]

All in all it is probably negligible (and more or less constant) so it is not a huge problem (and we could also seek to optimise the search at some point).

[Debilski]

For reference: aspp2022_3 does indeed break with this change:

Later in the code they do a random.choice(bot.legal_positions) on a then empty list which luckily raises an uncaught exception.

[otizonaizit]

OK, so we'll need to add the property back using the compatibility decorators!

On Tue 02 Jul, 02:19 +0000, Rike-Benjamin Schuppner @.***> wrote:

For reference: aspp2022_3 does indeed break with this change:

image.png (view on web)¹

Later in the code they do a random.choice(bot.legal_positions) on a then empty list which luckily raises an uncaught exception.

— Reply to this email directly, view it on GitHub², or unsubscribe³. You are receiving this because you authored the thread.☘Message ID: @.***>

––––

¹ https://github.com/ASPP/pelita/assets/216179/9a65872c-9e3f-4c9a-b54b-8b176dcf1fa5 ² https://github.com/ASPP/pelita/pull/786#issuecomment-2202489656 ³ https://github.com/notifications/unsubscribe-auth/AACUYC7TRDE323KY25T2XXLZKJWA5AVCNFSM6AAAAABKHEEJBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBSGQ4DSNRVGY

[Debilski]

Do you have an idea of how to patch this?

[otizonaizit]

Fixed in https://github.com/ASPP/pelita_private_bots/pull/10