NxRLab / ModernRobotics

Modern Robotics: Mechanics, Planning, and Control Code Library --- The primary purpose of the provided software is to be easy to read and educational, reinforcing the concepts in the book. The code is optimized neither for efficiency nor robustness.
http://modernrobotics.org/
MIT License
1.96k stars 815 forks source link

Book software bugs report in Chapter 4 #2

Open zjudmd1015 opened 7 years ago

zjudmd1015 commented 7 years ago

About book software of Chapter 4, in the function FKinSpace.m and FKinBody.m, the for loop uses (size(thetalist)) to count how many times it need to pre-multiply or post-multiply. However, (size(thetalist)) returns dimension of thetalist, which is a vector. Besides, for loop takes the first element of the vector there. As a result, times of loop depends on whether you input the thetalist as row vector or column vector, which is not robust enough and can be easy to made a mistake.

For example, The code in the FKinSpace.m is listed below.

for i = size(thetalist):-1:1
    T = MatrixExp6(VecTose3(Slist(:,i) * thetalist(i))) * T;
end

Here we have a thetalist = (th1, th2, th3, th4). If we input it as thetalist = [th1; th2; th3; th4]; , it will be a 41 column vector, and (size(thetalist)) returns [4, 1], so the output result will be right. Otherwise, if we input it as thetalist = [th1, th2, th3, th4];, it's 14 row vector, and (size(thetalist)) returns [1, 4], so the for loop only run one time, which cause a wrong output. One solution to make it more robust is to replace the size() function to length() function, which only returns a scalar noting length of whatever a row vector or column vector.

for i = length(thetalist):-1:1
    T = MatrixExp6(VecTose3(Slist(:,i) * thetalist(i))) * T;
end

Miaoding Dai _m.dai@u.northwestern.edu

jarvisschultz commented 6 years ago

I agree that this is a problem, but I don't agree with the proposed fix. Nearly every function in the library is sensitive to the shape of the input arrays. Randomly breaking this strict shape-dependence results in an inconsistent API. I think that real issue here is that if you pass the wrong shape for thetalist you get an answer, but not the correct answer. Likely the better approach to addressing this issue is adding more error checking to the library broadly, and coming up with a consistent strategy for handling errors (e.g. throwing exceptions, returning NaN/None/error flags, printing warnings, assertions, etc.).