One might think that what just happened was bad. In fact, I was recently reminded that it was quite normal.

Late yesterday, I discovered while making my parameterized test even more parameterized, that the handling of a wheel was wrong when the wheel was not at the (0,0) origin. In a fit of effort not fully recorded, I quickly found and patched the issue. We now have, I am confident, a set of routines that correctly manages all the cases I’ve thought of for the connecting rod calculations. We’ll be reviewing that code shortly. First, I want to summarize what happened.

My design idea, as often reported here, was that the calculation should be structured like this:

  1. Translate the given problem to the origin (0,0) if necessary;
  2. Rotate by the negative of the piston angle to get the motion axis horizontal;
  3. Solve the problem for horizontal motion;
  4. Rotate the result back to the piston angle;
  5. Translate the result back to its true origin.

That was, and remains, the plan. What I actually did was to first test-drive the central solution, #3 on the list above. Then I worked om dealing with the origin and test-drove that: an easy one, Then I test-drove the rotation part and plugged it in.

As it happens, all my rotation tests used a zero origin up until late yesterday. As soon as I tried a non-zero origin, tests failed. In essence, the code was doing step #5 and then #4, and that won’t work, because when we solve a problem by transforming it and transforming the answer back, we have to apply the reversing transformations in mirror order, ABC-CBA.

I certainly would have preferred to have done it right the first time, but in fact there is an important lesson here. GeePaw Hill mentioned it at last week’s Zoom session. I’ll put it in my own words: doubtless he would do better.

Our plans generally show a rather direct path from start to finish: first we’ll do this, then that, then the other thing, then the final thing. One might think that the work would, or should, go like that. One would be, not to put too fine a point on it, mistaken.

We want to program in very small steps, supported by tests, such that the code is never broken for very long at all. We don’t mean “just a few days”. We don’t mean “just a few hours”. We mean “just a few minutes”.

For me, my rough rule of thumb is that if I go for more than twenty minutes without getting back to all green, I am probably lost in the weeds.

It’s like this: the plan says “then we cross the little stream”. When we get to the little stream, it’s moving pretty fast, it’s rather deep, but there are some stones that look pretty stable. So we step from stone to stone, always trying to pick a move that we can make without getting doused. We wind up zig-zagging across the stream.

That’s if things go well. Sometimes the stone we step to tilts, or is slippery, or turns out to be a turtle, and we have to climb back up on our previous rock and try again.

The simple straightforward plan turns into a zig-zag path leading from where the plan starts to … where?

Well, in the simple case, it leads across the stream. And listen to me here, the zig-zag path was the best path across that we could find.

Sometimes it isn’t even that simple. We get half-way across and there are no more stones. Or downstream we see a log across the stream. Or upstream we discover a canoe. Sometimes we even discover that we don’t have to cross the stream at all.

We expect that our path through the development will not be the same as the plan suggests. When we’re actually at the stream, things are quite different from that little squiggle on the map.

My path from not having a connecting rod solution to having one didn’t follow the tests 1,2,3,4,5, because the stone that seemed easiest to step to was the origin one, not the rotation one, so I went there.

When I got close to the shore, I couldn’t quite make it, because stepping over to origin first jumbled the order of things. This time I found a couple of stones that got me to the shore, ones that backed out the origin, applied the rotation, and then put the origin back.

That’s just fine … as far as it goes.

I mean it: it’s just fine. We would do well to expect this kind of deviance from the plan, because it’s in the nature of programming to discover different paths, and our job is to choose among them, trying always to move roughly toward the goal.

But now we’ve found our across the stream, and it remains true that the ideal pattern of operation is 1,2,3,4,5, not whatever we have now, perhaps something like 1,2,3,5,1,4,5. What we need now, is a good way across the stream. It’s time to refactor.

Refactor to What?

Well, refactor to a better algorithm. I think it should follow the 1-5 above, but perhaps there is something better. We’ll review our code and start improving it. Here it is:

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    use_radians = -piston_radians + wheel_radians + lead_radians
    start, finish = con_rod_ends_with_origin(
        origin, radius, use_radians, length)
    s = start-origin
    f = finish-origin
    sr = s.rotate_xy(piston_radians)
    fr = f.rotate_xy(piston_radians)
    return sr+origin, fr+origin
    # return start.rotate_xy(piston_radians), finish.rotate_xy(piston_radians)

def con_rod_ends_with_origin(origin, radius, theta, length):
    start, finish = con_rod_ends(radius, theta, length)
    return start + origin, finish + origin

def con_rod_ends(crank_radius, crank_angle, length):
    # zero-based, horizontal
    crank_offset = vector(crank_radius,0).rotate_xy(crank_angle)
    constrained_end = con_rod_solve(crank_offset, length)
    return crank_offset, constrained_end

def con_rod_solve(crank_offset, length):
    x,y = crank_offset.x, crank_offset.y
    x_projection = math.sqrt(length*length - y*y)
    return VtVector((x+x_projection,0,0))

My patch is in con_rod_ends_complete, where I take the result of calling con_rod_ends_with_origin, factor out the origin, then do the rotation, then put the origin back in.

We have all the pieces here, and they are happening in the right order now, sort of. It seems to me that if con_rod_ends_complete just called con_rod_ends instead of con_rod_ends_with_origin, it wouldn’t have to do the sr and fr adjustment.

We have over a thousand tests. Let’s make that change. I make sure I’m on a clean commit.

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    use_radians = -piston_radians + wheel_radians + lead_radians
    start, finish = con_rod_ends(radius, use_radians, length)
    sr = start.rotate_xy(piston_radians)
    fr = finish.rotate_xy(piston_radians)
    return sr+origin, fr+origin

The tests all pass. Remove the _with_origin function, test again, and commit. Well, no. There are tests for that function. Leave it in until we’re sure we’re done. Do commit this change, as it is better and the tests all run.

I have a helper function, vector, to get around the awkward VtVector creation. I found that vector class somewhere, but I’ve had to modify it a bit.

def vector(x,y):
    return VtVector((x,y,0))

I use that in my function. There is just one test of the _origin function. Remove that and the function. We’re here:

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    use_radians = -piston_radians + wheel_radians + lead_radians
    start, finish = con_rod_ends(radius, use_radians, length)
    sr = start.rotate_xy(piston_radians)
    fr = finish.rotate_xy(piston_radians)
    return sr+origin, fr+origin

def con_rod_ends(crank_radius, crank_angle, length):
    # zero-based, horizontal
    crank_offset = vector(crank_radius,0).rotate_xy(crank_angle)
    constrained_end = con_rod_solve(crank_offset, length)
    return crank_offset, constrained_end

def con_rod_solve(crank_offset, length):
    x,y = crank_offset.x, crank_offset.y
    x_projection = math.sqrt(length*length - y*y)
    return vector(x+x_projection,0)

I think that the transform notion, my 1,2,3,4,5, is not obvious in this code. This is the case because:

  1. The inner calculations work from offsets, using radius and angle, so we never have to remove the origin. (But we do have to apply it at the end.)
  2. The removal of the piston angle is hidden in the use_radians calculation. (But quite visible in the rotate_xy calls.)

I would much prefer to make it clear how this function works, because someday someone is going to want to understand it, and that someone might be me. That said, while I think we could compute the absolute coordinates of the crank end, then remove the origin, then rotate them by -piston_radians etc., that’s just too extra.

Let’s try a couple of explaining variables. Maybe even some comments.

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    crank_radians = wheel_radians + lead_radians
    # rotate by - piston_radians
    effective_radians = crank_radians - piston_radians
    # calculate horizontal, origin-free.
    start, finish = con_rod_ends(radius, effective_radians, length)
    # rotate back by piston_radians
    sr = start.rotate_xy(piston_radians)
    fr = finish.rotate_xy(piston_radians)
    # translate to origin
    return sr+origin, fr+origin

I tried plugging in some methods to replace the comments, but the result seemed worse. One more idea: I’ll try again. I think I might go with this:

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    # translate to origin not needed
    effective_radians = rotate_to_horizontal(wheel_radians, lead_radians, piston_radians)
    start, finish = con_rod_ends(radius, effective_radians, length)
    fr, sr = rotate_inverse(start, finish, piston_radians)
    return translate_inverse(sr, fr, origin)

def rotate_to_horizontal(wheel_radians, lead_radians, piston_radians):
    crank_radians = wheel_radians + lead_radians
    return crank_radians - piston_radians

def rotate_inverse(start, finish, piston_radians):
    sr = start.rotate_xy(piston_radians)
    fr = finish.rotate_xy(piston_radians)
    return fr, sr

def translate_inverse(sr, fr, origin):
    return sr + origin, fr + origin

def con_rod_ends(crank_radius, crank_angle, length):
    # zero-based, horizontal
    crank_offset = vector(crank_radius,0).rotate_xy(crank_angle)
    constrained_end = con_rod_solve(crank_offset, length)
    return crank_offset, constrained_end

def con_rod_solve(crank_offset, length):
    x,y = crank_offset.x, crank_offset.y
    x_projection = math.sqrt(length*length - y*y)
    return vector(x+x_projection,0)

I like that it expresses the algorithm a bit better. However there is one more thing that we should really look at. What if we were to inline everything?

def con_rod_ends_complete(
        *, piston_radians, origin, radius,
        lead_radians, wheel_radians, length):
    crank_radians = wheel_radians + lead_radians
    effective_radians = crank_radians - piston_radians
    crank_offset = vector(radius, 0).rotate_xy(effective_radians)
    x, y = crank_offset.x, crank_offset.y
    x_projection = math.sqrt(length * length - y * y)
    constrained_end = vector(x + x_projection, 0)
    return (origin + crank_offset.rotate_xy(piston_radians),
            origin + constrained_end.rotate_xy(piston_radians))

Tests are still all green. I did all those changes using PyCharm’s refactoring tools, except for moving origin to the front of the final return expressions.

Wow. 12 lines, or 32? Which is better? Could someone figure out how that works? Would it be easier or harder than with the larger but presumably more expressive version? What would you pay for a drawing if you had to work out how that works? I honestly do not know.

What I’ve done for now is roll back to the version before the inlining, and then pasted the inline function in with a new name, because I kind of want to see it in there for a while.

The Point

Well, of course one point is that we seem to have the connecting rod problem solved, with over 3000 tests exercising and verifying the code. I’m almost convinced that it works.

But the more general point might be that while it is good to have a plan, we should expect and welcome small deviations from that plan as we work, and we should be confident that when we do get to the solution, we’ll be able to refactor it into whatever shape we deem best, whether that’s an open expressive form, or some tight inlined code.

As Hill puts it, “We are in the business of changing code”. With practice and good tools, we can do it rather well.

Next time, I almost promise, we’ll put this into a ConRod object and draw some pictures. Unless a more interesting path presents itself.

See you then!