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

Auto normalizing positions for python starter kit #176

Merged
merged 4 commits into from Dec 9, 2018

Conversation

Projects
None yet
4 participants
@coreylowman
Copy link
Contributor

coreylowman commented Dec 7, 2018

This closes #78.

Changes

This PR adds WIDTH and HEIGHT to constants, and adds a new normalize method to Position. This allows positions to be normalized without having to use the game map.

Further, Position is then modified to auto normalize itself in the constructor.

Motivation

I believe this is necessary because of the consequences of not auto normalizing Positions. As I'm sure many of you are aware, Path Planning algorithms depend heavily on not expanding positions they've already evaluated. But, as a consequence of not auto normalizing positions, naive use of the Position class will result in much more work than expected. This is because Position equality and hashing does not take into account the width and height of the game. This leads to this effect in a 32x32 board:

>>> Position(0, 0) == Position(32, 32)
False
>>> game_map.normalize(Position(0, 0)) == game_map.normalize(Position(32, 32))
True

Consider this simple breadth first search code:

def bfs(start: Position, goal: Position):
    explored = set()
    open = {start}
    while len(open) > 0:
        yield open

        next = set()
        for position in open:
            for neighbor in position.get_surrounding_cardinals():
                if neighbor not in explored:
                    next.add(neighbor)
                    explored.add(neighbor)
        open = next

This may actually cause an infinite loop with the current behavior. Think about what would happen if start was at (0, 0). The neighbors generated in the first iteration would look like:

{(1, 0), (0, 1), (-1, 0), (0, -1)}

Note that since we aren't normalize, there are Positions with negative xs and ys in that set. Now think ahead a bit to when the code reaches (32, 32), which will happen because positions are not normalized. Now we know that (0, 0) is exactly the same square as (32, 32), but the hashing function does not realize this. It sees that (32, 32) is not in the set of explored positions, so it adds it to the set so it can explore it. Now we will explore the exact same squares we've already explored, but with their xs and ys above 32. This causes an infinite loop.

Here is the correct code (as of now):

def bfs(game_map, start: Position, goal: Position):
    explored = set()
    open = {game_map.normalize(start)}
    while len(open) > 0:
        yield open

        next = set()
        for position in open:
            for neighbor in position.get_surrounding_cardinals():
                neighbor = game_map.normalize(neighbor)
                if neighbor not in explored:
                    next.add(neighbor)
                    explored.add(neighbor)
        open = next

Of course, this code isn't that much different, and one could argue that it isn't hard to call normalize(), but I would argue that this subtlety of having to normalize positions isn't stressed enough, or at all in the docs. How is someone going to know that they have to do that? They won't.

This happened to me the other day... I was confused why my bot was timing out sometimes during my A* algorithm, because A* is pretty fast. I come to find out that its evaluating positions that have coordinates like (-92, 48) and things like that.

Conclusion

Anyone who isn't using exclusively normalized positions is probably doing a lot of extra work or timing out more often than they should be, because any equality check or hash check on un-normalized positions will not work as expected.

Note that this is a problem with every starting kit that doesn't auto normalize positions, but this PR only fixes the python start kit.

@snaar

This comment has been minimized.

Copy link
Contributor

snaar commented Dec 7, 2018

This does not close #78, since that is about game engine modification to send map width/height via json constants. However this does help with doing the changes that will be needed once those are populated. So I'm okay with this change as-is and once #78 is out, we can update python kit to get constants from the json blob.

@@ -62,10 +63,17 @@ def invert(direction):


class Position:
def __init__(self, x, y):
def __init__(self, x, y, normalize=True):

This comment has been minimized.

@snaar

snaar Dec 7, 2018

Contributor

I think we should always normalize. Performance penalty is trivial compared to all the other horrible things python has.

@snaar
Copy link
Contributor

snaar left a comment

Need to normalize in iadd and isub too.

@@ -36,6 +36,8 @@ def __init__(self):
self.me = self.players[self.my_id]
self.game_map = GameMap._generate()

constants.set_dimensions(self.game_map.width, self.game_map.height)

This comment has been minimized.

@snaar

snaar Dec 7, 2018

Contributor

Can you add comment "Remove once width/height are sent by the server (#78)."

@@ -58,3 +58,9 @@ def load_constants(constants):

"""An inspired ship instead spends 1/X% halite to move."""
INSPIRED_MOVE_COST_RATIO = constants['INSPIRED_MOVE_COST_RATIO']


def set_dimensions(width, height):

This comment has been minimized.

@snaar

snaar Dec 7, 2018

Contributor

Can you add comment "Remove once width/height are sent by the server (#78)."

@snaar snaar merged commit f8c5dbb into HaliteChallenge:master Dec 9, 2018

@bastiankayser

This comment has been minimized.

Copy link

bastiankayser commented Dec 11, 2018

When starting the python starterkit (Halite3_Python3_Windows-AMD64) I get the following error:
File "MyBot.py", line 23, in
[error] [1] [P0] game = hlt.Game()
[error] [1] [P0] File "D:\Projekte\Python_Meetup\Halite3_Python3_Windows\hlt\networking.py", line 36, in init
[error] [1] [P0] self.players[player] = Player._generate()
[error] [1] [P0] File "D:\Projekte\Python_Meetup\Halite3_Python3_Windows\hlt\player.py", line 64, in _generate
[error] [1] [P0] return Player(player, Shipyard(player, -1, Position(shipyard_x, shipyard_y)))
[error] [1] [P0] File "D:\Projekte\Python_Meetup\Halite3_Python3_Windows\hlt\positionals.py", line 70, in init
[error] [1] [P0] self.normalize()
[error] [1] [P0] File "D:\Projekte\Python_Meetup\Halite3_Python3_Windows\hlt\positionals.py", line 73, in normalize
[error] [1] [P0] self.x = self.x % constants.WIDTH
[error] [1] [P0] AttributeError: module 'hlt.constants' has no attribute 'WIDTH'

which is reasonable since the call to set_dimensions(width,height) comes after the call to positionals, see networking.py l.35 and netowring.py l. 40

Bug or did I make a mistake here?

@coreylowman

This comment has been minimized.

Copy link
Contributor Author

coreylowman commented Dec 11, 2018

woops, that's a bug. that's my bad.

@snaar this could be fixed by adding the normalize option in the constructor back in. Then in Player._generate and GameMap._generate, pass in normalize=False to the Positions initialized there. Seems kind of hacky though.

Another option is to remove self.normalize() from Position.__init__.

Since we don't have width and height until after GameMap is initialized, I'm not sure what other way that would be resolved, since Positions are initialized before in Player._generate.

Maybe this work-around should just be reverted until #78 is completed?

@lidavidm lidavidm referenced this pull request Dec 11, 2018

Merged

Fix Python starter kit #181

@lidavidm

This comment has been minimized.

Copy link
Contributor

lidavidm commented Dec 11, 2018

@bastiankayser thanks for the report, a fix is in #181 which I'm hoping to merge+deploy soon. I've attached the kit here.
py3.zip

@bastiankayser

This comment has been minimized.

Copy link

bastiankayser commented Dec 13, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment