The Repo on GitHub

Last night, we shared some observations about the code. We shared some observations about how we pair. We even did a bit of nice work.

Mostly we just chatted last night, but at one point we were showing Bryan, who has been away from the code for a bit, what was going on. GeePaw drove during that.

Pairing Feedback

As we three worked on the code, GeePaw drove and Bryan and I tried to help. GeePaw pushed back a few times and at the time I recall thinking he was just grumpy. After the coding finished, he gave us some feedback on the session. This took the form of a very gentle rant to the effect that he is actually a very good programmer, and he does know Python and PyCharm rather well, and it felt to him as if we thought he was a newbie and were telling him what to type rather than what we wanted to accomplish. Of course we do not think he’s a newbie, but yes, we were often telling him things like “you forgot self” or “tab the next line out also”.

In our defense, using me as the example because I’m here now, when I am typing a program I will often fail to type self, and I’ll say to myself, often even aloud, “you forgot self”, which reminds me to go back and do it. I will often notice “oh the next line has to tab also”, which reminds me to do it. So what GeePaw was hearing as disrespect and criticism was really more likely just our stream of thought as things happened.

This is not to say that GeePaw should just “get over it”, although the thought does come to mind, but I think it does offer some opportunities for people pairing. The helper might do well to just offer feedback less often, less immediately, because the self will either be fixed or it’ll still be missing in a few seconds, in which case maybe it’s worth mentioning. And the group, driver and helpers, might want to devise ways to just say “a little less feedback on typos”, or something like that. Perhaps some agreed code words?

When pairing in person we get a lot more feedback. I used to like to sit across the table from Chet, like this:

  C
===
R

That setup lets the pair look at each other when chatting, which is nice for some of us, and makes it easier to see any posture changes or fidgeting that could be cues about how things are going. On Zoom, the IDE fills the whole screen and you’re lucky if there are even small pictures of the typists, so it is very hard to get the same level of feedback.

For my own improvement, I’ll just try to speak less often, and in higher-level terms when I do. That is a bit problematical, because last night I almost stopped offering any feedback sometimes because of what I perceived as grumpiness. I quickly fell back into my old ways, of course, increasing the tension all unknowing. I take the feedback to heart: I was not offended by it in any way, in the moment or now. I’ll try to take it on board and adjust my behavior, just as I try to take programming mistakes on board and try to improve from them.

Code Issue

One thing that troubles all of us is this code in the Bot:

When it is time for a Bot to take its turn, it is to be sent do_something. Perhaps not the best name. Presently only tests and the code that runs and displays bots on the screen, game, ever calls do_something. We all find the method’s code to be confusing:

    def do_something(self):
        self.update_knowledge()
        self.state = self.state.update(self._knowledge)
        self.do_state_actions()
        self.move()

Why does that confuse us? Let me count the ways. First of all, the conventional way a state machine works is that first you call action and then you update to a new state. Here, we update first, so the inquiring mind wants to know why. There is nothing in the code that explains why.

Why?
I chose to do it this way for reasons one could object to: preparation for the future. The intention with this program was—and I guess still is—for it to be a client-server setup when multiple clients can field multiple robots each, and have them operate in the world. Our grand initial idea, which is in my view rapidly crumbling down to a grudging form more in tune with reality, was that different teams would devise different bots, and that they would collaborate in the world to mutual benefit. As things stand now, we’ll be lucky to have even one client.

Be that as it may, please pardon my grumbling, the intended layout will almost certainly be something like a client, either upon signal or just on its own recognizance, will send a batch of commands, for all its bots, to the server. In due Internet time, that client will receive an update containing all the new information about those bots, depending on what happened to them in the world.

That says to me that there will be a higher-level object, say Client, that will create a bunch of bots, connect to the server, and begin cycling. So the overall flow will be something like 1) accumulate actions for each bot, 2) send the actions to the server, 3) receive the results, 4) update the bots with their new Knowledge. The code as written, with update before action is there to reflect that situation. I felt, and still feel, that doing it in the more conventional order is just asking for trouble. My colleagues are not so sure.

One thing is particularly confusing as things stand now: suppose you’re in a state. You choose some actions. You return your action list and the do_something issues them as commands to the world. You’re done for now. Then next time around, the info is back, and your state changes because you used to be Looking and now you have a Block so you transition to Laden. And the first thing that happens in Laden is its action.

Oh Wow
I just got a wild idea what could be done about this. We could perhaps use a Future object, or a Promise. Heavy. High tech. But an interesting idea.
Slightly Less Weird
Perhaps less weird would be to use the sequence action - update in the conventional order, and have the Bot assume what the net state will be … and then, if that assumption is incorrect, fix it up next time around. So the sequence in do_something might be 1) correct state if needed, 2) take state action, 3) set new state. Must think about that.

But I digress.

We do not have a proposed fix for this confusion but we were faced with it last night as Bryan struggled with it and both Hill and I tried to explain it. It’s just strange and hard to think about.

We do think that moving to a more explicit expression of the client-server form would perhaps help. For now, it’s just hard to understand.

Some actual work

We found a bit of work to do. The Knowledge had a member variable tired, which was counted down in the Bot and set to a positive value in the state changes where the bot picked up something or dropped it. The bot then went to Walking state and stayed there for a few turns, until tired had been counted down to zero, at which point it would go to Looking or Laden, whichever was suitable. The whole point of that was to cause the bot to (probably) move away from where it had picked up or dropped off, so that it was less likely to drop off or pick up in the same place, to provide more variability.

We had all long agreed that the tired concept wasn’t quite if, but had not had an idea what to do about it. I think it was Bryan who suggested that the Bot had “energy” and that when it had done some picking up or dropping off, it lost energy and had to build it back up before it went back to work.

We modified Knowledge accordingly:

class Knowledge:
    drop_threshold = 4
    take_threshold = 7
    energy_threshold = 5

    def __init__(self, location, direction):
        self._old_location = None
        self._location = location
        self._direction = direction
        self._vision = Vision([], self.location, self.direction)
        self._entity = None
        self._energy = self.energy_threshold
        self.scent = 0

    def has_energy(self):
        return self._energy >= self.energy_threshold

    def use_energy(self):
        self._energy = 0

    def gain_energy(self):
        self._energy += 1

We settled on a particular energy threshold, since we usually had used 5 as the amount of tired to give the bot, but reversing the sense and incrementing up to the threshold just makes a lot more sense. And at GeePaw’s behest, we encapsulated all the literals and direct setting of the member into the three methods shown, used like this, for example:

class Bot:
    def do_something(self):
        self.update_knowledge()
        self.state = self.state.update(self._knowledge)
        self.do_state_actions()
        self.move()

    def update_knowledge(self):
        self._knowledge.gain_energy()  # <===


class Walking:
    def action(self, _knowledge):
        return []

    def update(self, knowledge):
        if knowledge.has_energy():  # <===
            if knowledge.has_block:
                return Laden()
            else:
                return Looking()
        return Walking()


class Looking:
    def action(self, knowledge):
        if knowledge.can_take:
            return ['take']
        return []

    def update(self, knowledge):
        if knowledge.has_block:
            knowledge.use_energy()  # <===
            return Walking()
        else:
            return Looking()

class Laden:
    # similarly

So that’s a lot nicer than it was. And that’s my report on last night.

One More Thing

GeePaw also remarked that if this was his code base he’d sort out the tests, because why aren’t tests for Knowledge in a file called test_knowledge and a class called TestKnowledge. I admit that even with my greater familiarity with the code, I find tests hard to find sometimes. And I think there are some tests that are too big, and could be much more micro.

I’m not as good at that as GeePaw is, but I can certainly do some renaming.

There is a file called test_class_per_state_machine.py. It contains TestClassPerStateMachine class. Let’s change that to refer to um … state_machine? And probably rename the file containing the machine to state_machine?

Or should we just call it machine, since we’re used to that. Yes, we’ll go that way.

Along the way I discover that although I have a FakeMachine class, it is not used. So I reemove references to it and the class itself.

test_machine.py
class TestMachine:
    ...

Commit: rename machine tests to test_machine.py and TestMachine.

The file test_decisions is all about tests for Knowledge. Rename. Commit: rename TestDecisions to TestKnowledge.

The file test_cs_spike was something I did that seems to mean nothing now. Remove it. It’ll be in history if we ever need it. Commit: remove test_cs_spike.

There’s a file test_pathing that includes two tests for Bot that may be work keeping. I move them to the test_bot file. The test of test_pathing is a little spike I did while thinking about sending a bot to some new location over several turns. Not needed, easy to redo or get back. Remove it. Commit: move tests and remove test_pathing.

There are a lot of references to Bot in test_location. I mean a lot. I move them. Commit: move bot tests from test_location to test_bot

That’s about all I can see at a quick glance. Let’s wrap it up and get out of here.

Summary

Some good feedback from last night. Took a bit of action to clean up the tests, file naming, and test locations. Some mornings, that’s about as deep as you want to go. This is one of those mornings. See you next time!