Don't Move!
Moving toward everything running on
start/finish, I think we need a place to stand. And not on a wheel. HOWEVER: Plan Dashed Immediately.
It has been a day or so since I looked at the code, so I’m not sure quite where we stand vs the plan to have all the components rely on start / finish parameters to update, and return their own start / finish as their response to that method call. So we’ll have a look.
Today’s plan going in is to introduce a new component, FixedPoint, which is nothing more than a cover for a … well … for a point. Presently, the Wheel’s parent is a vector, I believe, and it seems to me that in principle, there could be a wheel whose center moved, or some other kind of object with a fixed end. So I want the FixedPoint. I don’t think it’s premature to do that: it feels right to me. We’ll see.
In we go, looking around to see how the start / finish changes are going., If they’re going well, none of the components will be accessing their parent member any more, relying only on their start and finish parameters to update.
Immediately I am disappointed. PyCharm was open on ConRod, which includes this:
class ConRod:
def update(self, start, finish):
self._start, self._finish = (
con_rod_ends_complete(
piston_radians=self._piston_radians,
wheel_center=self.crank_origin(),
radius=self.crank_radius(),
crank_radians=self.crank_radians(),
length=self._length))
return self._start, self._finish
A subtle hint, in case I didn’t notice the details of the con_rod_ends_complete call, is that in PyCharm, start and finish are a pale gray, signifying that they are never referenced. Meh.
Belay the FixedPoint plan. Compared to this, it’s a refinement. We need to refactor our code to use start and finish to compute all the values we need. OK. Check the methods called from update, fixing each one in turn.
class ConRod:
def crank_origin(self):
return self._crank.origin()
I reckon that’s start but we’ll look at Crank to be sure:
class Crank:
def origin(self):
return self._parent.start
Dammit! OK, but what is Crank’s own start? Ah, life is good:
class Crank:
@property
def start(self):
return self._parent.start
OK, so we could pass start down to the crank_origin method but let’s just use start directly in setting wheel_center. Tests should tell us if that’s a mistake.
class Crank:
def update(self, start, finish):
self._start, self._finish = (
con_rod_ends_complete(
piston_radians=self._piston_radians,
wheel_center=start,
radius=self.crank_radius(),
crank_radians=self.crank_radians(),
length=self._length))
return self._start, self._finish
Tests pass. I want to see the drawing. This tells me that I don’t fully trust my tests. I know that: it’s why I built the drawing. I’d rather trust the tests and should think about why I don’t. Drawing is good. Before I commit, can I remove that origin method? There seems to be another caller. It’s in draw, which has start, so I change that line as well. Remove the method, test. Green. Commit: refactoring ConRod to use only start / finish parameters.
While doing that, I note that the init_draw is referencing the cached start and finish, and before we’re done, we should be passing those values to that method and the draw method as well. Note made. One thing at a time.
Next in update, crank_radius:
class ConRod:
def crank_radius(self):
start, finish = self._crank.start_finish()
return finish.distance(start)
We could inline that, but let’s leave the method for clarity. Fix:
class ConRod:
def update(self, start, finish):
self._start, self._finish = (
con_rod_ends_complete(
piston_radians=self._piston_radians,
wheel_center=start,
radius=self.crank_radius(start, finish),
crank_radians=self.crank_radians(),
length=self._length))
return self._start, self._finish
def crank_radius(self, start, finish):
return finish.distance(start)
Green. Commit, same message. Next is crank radians:
class ConRod:
def crank_radians(self):
start, finish = self._crank.start_finish()
diff = finish - start
return math.atan2(diff.y, diff.x)
Same treatment, resulting in:
class ConRod:
def update(self, start, finish):
self._start, self._finish = (
con_rod_ends_complete(
piston_radians=self._piston_radians,
wheel_center=start,
radius=self.crank_radius(start, finish),
crank_radians=self.crank_radians(start, finish),
length=self._length))
return self._start, self._finish
def crank_radius(self, start, finish):
return finish.distance(start)
def crank_radians(self, start, finish):
diff = finish - start
return math.atan2(diff.y, diff.x)
We’ll have to keep caching those results until we come back and work on the drawing. But we’re green so commit: ConRod calculations now rely solely on start / finish parameters to update method.
I should mention that I did those two refactorings with PyCharm’s Change Signature, providing the field names start and finish and the same as the default values. When PyCharm changed the calls, it referred to start and finish as the values, which are, of course, the names of the parameters to update, so it all links up. Very nice.
What Next?
We could return to Plan A, leading to FixedPoint, or we could divert and add start and finish parms to the drawing methods. I think the latter is a bigger deal, as it will impact multiple classes, unless I come up with a clever idea, so we’ll defer that and keep checking our existing components for proper use of start / finish, which really should be done before FixedPoint.
Doesn’t have to be, of course, since FixedPoint will only be used in Wheel class at present. Still, let’s stay on the main thread, getting converted to use the parameters, not cached values and definitely not the parent.
So. ConRod is done. How about Piston? It should be easy if not done.
class Piston:
def update(self, _start, finish):
self._start = finish
finish_offset = vector(self._length,0).rotate_xy(self._radians)
self._finish = self._start + finish_offset
return self._start, self._finish
That’s using and returning start / finish, still caching. We’ll chase caching when the start / finish and drawing are all done. Moving on to Crank.
class Crank:
def update(self, p_start, p_finish):
dif = p_finish - p_start
parent_radians = math.atan2(dif.y, dif.x)
return self.start, self.finish
We’re good. What about Wheel?
class Wheel:
def __init__(self, *, parent, angle_degrees=0):
self.line = None
self.circle = None
self._position = parent
self._angle_radians = angle_degrees * math.pi / 180.0
def update(self, start, finish):
self._angle_radians = self._angle_radians + 4*math.pi / 180.0
return self.start, self.finish
@property
def start(self):
return self._position
@property
def finish(self):
return self._position + vector(1, 0).rotate_xy(self._angle_radians)
Ah. This is why I wanted FixedPoint. The Wheel returns its position (a vector) as start, and a point at radius 1 positioned at the wheel’s current angle of rotation.
In our current loops over components, we are passing None to Wheel:
s = f = None
for component in components:
s, f = component.update(s, f)
component.init_draw(canvas)
We want the FixedPoint to be at the root of things, and for it to return its position as both start and finish.
This is going to be a bit tricky, because we have lots of creations of Wheel, and they all need to be converted.
I’m going to take a chance here, and create FixedPoint without tests, instead relying on existing tests and compile failures to test it. This is risky, but I’m in a rush for some reason. Let’s see what happens.
class FixedPoint:
def __init__(self, position):
self._position = position
def update(self, start, finish):
return self._position, self._position
I build the class inside ‘wheel.py’ for now and will move it soon. Getting a bit sloppy here? You could say that.
Back in wheel, we will have no use for the parent. (Soon, no one will have. Maybe mostly no one needs it now.) I’m not even sure why we need the angle parameter. We do have a couple of tests that use it. Here are all the wheel constructor calls:
I’ll just tick through those in a Find window.
I replaced all the vectors v with FixedPoint(v). Tests fail. Might should roll back, but I think I see what’s up.
I thought Crank was all nicely solved but it isn’t. It still has start and finish method. More to the point, someone is calling them. Ah. The tests aren’t working with the start / finish scheme. They rely on calling the start and finish properties, like this:
def test_basic(self):
center = FixedPoint(vector(0, 0))
radius = 1
length=5
tilt_angle = 0
lead_angle = 0
angle_degrees = 0
wheel = Wheel(parent=center, angle_degrees=angle_degrees)
crank = Crank(parent=wheel, radius=radius, lead_degrees=lead_angle)
con_rod = ConRod(parent=crank, length=length, piston_angle=tilt_angle)
con_rod.update(crank.start, crank.finish)
target = con_rod._finish
assert target == vector(6,0)
That was all very well when we allowed references to the parent but we’re moving away from that. Let’s see if I can make this one pass properly, according to the new scheme.
OK, That’s Not Gonna Work.
I think this is a valid test for when things are working, which they currently are not. I think the test is a good tests and the code’s not ready.
def test_basic(self):
radius = 1
length=5
tilt_angle = 0
lead_angle = 0
angle_degrees = 0
center = FixedPoint(vector(0, 0))
s,f = None, None
s,f = center.update(s, f)
wheel = Wheel(parent=center, angle_degrees=angle_degrees)
s,f = wheel.update(s, f)
crank = Crank(parent=wheel, radius=radius, lead_degrees=lead_angle)
s,f = crank.update(s, f)
con_rod = ConRod(parent=crank, length=length, piston_angle=tilt_angle)
s,target = con_rod.update(s, f)
assert target == vector(6,0)
We just go along updating and passing the results to the next update. However, the test still fails, because there are still calls to the start / finish methods in the component classes. I need to roll back and work more incrementally, somehow.
I’ll roll back and then paste the above test back in as a fail marker.
That’s done. Git Reset Hard, paste in the test, paste FixedPoint back in. One test failing, nothing committed. Good starting point for next time.
Summary
We’ve made some good small steps toward our objective of having the components rely only on their update parameters, with no references to their parents. However, we’ve discovered that things aren’t quite as simple as we’d like, because of our earlier reliance on start and finish properties, which do calculations involving the parent object, or perhaps referring to cached values that aren’t present. I honestly don’t know all the cases: we’ll find them and deal with them one at a time.
The knowledge is in the code, not in my head. I like it that way. Leaves more room for daydreams and such.
We’ll look at that one failing test and see what it takes to make it work. We may have to try a time or two to find small steps. This morning, 50 minutes elapsed before I reset. Too long.
As for not TDDing the FixedPoint … so far it appears to work fine. That sloppiness was not the cause of this pause. It’s just that the code isn’t ready to do what I asked of it.
See you next time!