ASPP / pelita

Actor-based Toolkit for Interactive Language Education in Python
https://github.com/ASPP/pelita_template
Other
63 stars 68 forks source link

Add new `Bot.graph` attribute #789

Closed otizonaizit closed 5 months ago

otizonaizit commented 8 months ago

After several years of experience with the new Bot API it is clear that almost all players sooner or later use the pelita.utils.walls_to_graph function to generate a graph representation of the maze.

In the demo bots we show how to store this into state.

Inexperienced programmers tend to overlook the fact that it is expensive to create the graph, and prefer to just do it at every turn instead of trying to understand how to use state.

My suggestion is to just bite the bullet and make Bot.graph a default attribute for the Bot object. It can be created once at the beginning of the game and cached.

This will remove the need of using state for something which is actually not different than bot.walls in terms of use.

Debilski commented 8 months ago

The API promise was so far that all attributes on Bot are reset at each step (not quite true for the cached attributes like homezone but there is no good reason for a user to modify them) and for everything else one would need the state dict.

The difference now is that the networkx graph is mutable and meant to be mutated, which makes it hard to reason about it. Code like the following will subtly break:

# scenario: team was divided in two subgroups working on different strategies
def move(bot, state):
    import team_a, team_b

    if state == {}:
        state[0] = init_state()
        state[1] = init_state()

    # evaluate strategy of working group a
    state_copy = deepcopy(state)
    next_pos_a = team_a.move(bot, state_copy)

    # evaluate strategy of working group b
    state_copy = deepcopy(state)
    next_pos_b = team_b.move(bot, state_copy)

    # heuristic that decides which of the moves is better
    return better_move(bot, next_pos_a, next_pos_b)

This code now breaks if team_a.move makes any changes to bot.graph.

(Actually, this code will also break now that #786 has been implemented. I didn’t think of this but I think this can actually become an unforeseen problem.)

I guess we could store the graph in some hidden attribute and have a function bot.graph() that makes a copy? Or simply have a data structure like an adjacency list that makes creating the graph less costly?

otizonaizit commented 8 months ago

Well, we can protect all attributes by making them read-only properties. So they will get an error when they try to do anything with it. Or, for graph, we can just use this: https://networkx.org/documentation/stable/reference/generated/networkx.classes.function.freeze.html I think the problem of mutability is easy to explain in the docs if we can't find a nice fix.

But, by the way, deepcopy(state) is science fiction: I have never seen it used in code from our students.

Debilski commented 8 months ago

But freezing means that the weights can still change, I think this isn’t sufficient. And people reading the docs is also sci-fi. :)

otizonaizit commented 8 months ago

Yes, ok. I mean, let's create the graph at the beginning, attach a copy to Bot and if they mess it up they'll find out, and we can point them to the docs if this happens. I still find this better than the current situation, where lots of people are simply regenerating the graph over and over again. And semantically it kind of makes sense to attach it as a static attribut to Bot, just like we do with Bot.walls.

On Wed 28 Feb, 03:57 -0800, Rike-Benjamin Schuppner @.***> wrote:

But freezing means that the weights can still change, I think this isn’t sufficient. And people reading the docs is also sci-fi. :)

— 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/issues/789#issuecomment-1968828438 ² https://github.com/notifications/unsubscribe-auth/AACUYC23OPMV3T2FZITRZ2TYV4LSZAVCNFSM6AAAAABD5VO7SGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRYHAZDQNBTHA