More technical suggestions #319
Conversation
Got the following error when trying to visualize the block diagram: Execution error NameError: name 'RenderDiagram' is not defined Adding path to utils fixed it and displayed the proper diagram.
Expected behavior: ICP passes grade book test. Observed behavior: ICP gets stuck at local minima. [Might be getting too liberal with my commits by editing the grading script.] [I was only able to test this by copying definitions into my personal notebook. So this commit should be tested by running the grade book on a known good implementation of ICP.] - Changing seed values to avoid getting stuck at a local minima (assumes that the exercise is not about augmenting the algorithm to avoid local minima, i.e. by testing several start orientations). - Increase number of iterations to allow more time to converge. - Shrink stopping tolerance to allow more time to converge. - Increase allclose tolerance to be more lenient.
The disc is supposed to be near the middle of the bunny, and instead, it was below all of the points. The default center is perfectly fine.
q_sol initializes the simulation at whatever the static equilibrium solution was. initializing with initial guess allows for the simulation to play solve with the physics engine.
I figure that if you go through all effort to give us so much help on the rest of the problem, then you probably don't mind adding a point cloud so we can see if out answers actually make sense. But.... I might be way overstepping with this modification.
I replaced the url with the permalink to the image in your github, but I am just running Jupiter online, so I can verify the update works properly.
Ok, might be a bit of hubris, but: I think there is a typo in the IRIS paper (which is then implemented in the code and leads to a mistake... I think.)
The paper typos are in equation 5, which trickles into equation 6.
The paper (eq. 5): E = {x s.t. (x - d).T * C_inv * C_inv.T * (x - d) = 1}
Me: E = {x s.t. (x - d).T * C_inv.T * C_inv * (x - d) = 1}
Justification:
E = {C*x + d, given |x| = 1
let u = C*x + d
x = C_inv * (u - d)
|x| = 1
|C_inv * (u - d)| = 1
x (and thus C_inv * (u - d)) is a column vector, so:
|C_inv * (u - d)| = (C_inv * (u - d)).T * (C_inv * (u - d))
= (u - d).T * C_inv.T * C_inv * (u - d))
More evidence that something is wonky:
I plotted the solution against what I came up with and there are a few tangent planes (namely the first and the last two in the linked video below) that intersect with the triangle that has the current "closest point". This seems wrong since it implies that the ellipse touches the triangle in a non-tangent manner (which in turn implies that there is a closer point than what is returned).
https://drive.google.com/file/d/1BxLxK-sO7y2qeN0eWxrEXEhZo4aL6TET/view?usp=sharing
But, I might be missing something.
Wow! I am getting more cocky with each new commit! Wasn't sure the best way to leave feed back that I was only able to get chamfer_dist to 0.003, but that was by lower the threshold to 142 from 150. I guess I am being lazy since there are points that satisfy the threshold dist of 150 but clearly aren't part of the mustard bottle. So I could probably write something to prune those points away... but it seems that most of the problems so far only ask for a proper implementation of whatever algorithm is presented, in this case using the heat map to generate a point cloud for the grasp.
There was a problem hiding this comment.
Sorry for the long delay, and thanks for opening the PR. I looked at this right when you opened it, but there were enough small problems that I put it on the backburner.
I've gone through the first bunch now, and will finish soon.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @Jakeisthesnake)
-- commits line 14 at r2:
our solutions ICP code already passes the grade book test. Without evidence showing me that ours is too brittle, I don't think I should be mucking with the tolerances.
-- commits line 34 at r4:
that's also interesting. but not the authors' original intent.
book/pose/exercises/ransac.ipynb line 208 at r3 (raw file):
"plane_equation = fit_plane(bunny_w_plane)\n", "print(plane_equation)\n", "DrawFacet(plane_equation, \"naive_plane\", center=[0, 0, -plane_equation[-1]])"
thanks. the entire drawing code was a mess (and I think buggy!); i've been meaning to do a complete clean up and and done that now.
book/robot/exercises/01_reflected_inertia.ipynb line 46 at r1 (raw file):
" TestSimplePendulumWithGearbox,\n", ")\n", "from manipulation.utils import RenderDiagram\n",
That import is already available here.
book/trajectories/exercises/taskspace_iris.ipynb line 319 at r8 (raw file):
" \n", "Here is the pseudocode for this algorithm from the IRIS paper:\n", "<img src=\"https://github.com/RussTedrake/manipulation/blob/44fbe1e939d03e351fe91493e9779958eb19b097/book/figures/exercises/iris_sep_hyperplane_opt.png\" width=\"500\"> \n",
thanks. i've fixed it (using the raw.github...)
book/clutter/exercises/normal_estimation_depth.ipynb line 89 at r5 (raw file):
" cloud = point_cloud.Crop(lower_xyz=[-0.3, -0.3, -0.3], upper_xyz=[0.3, 0.3, 0.3])\n", " meshcat.SetObject("point_cloud", cloud)\n", " AddMeshcatTriad(meshcat, "least_squares_basis", length=0.03, radius=0.0005)\n",
that seems very reasonable to me. I've taken this change (but i have to take it in the upstream, private, solutions repo, and then "install it" into this repo.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 1 of 1 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Jakeisthesnake)
-- commits line 57 at r9:
yes. sadly, that's a typo that I'm aware of. the implementation is correct Drake, but i let the incorrect version slip in here... thanks for catching it.
N.B. I also completely rewrote the test.
book/segmentation/exercises/segmentation_and_grasp.ipynb line 420 at r16 (raw file):
"The two most important elements for our task are `labels`, which tells us which class the mask corresponds to, and `mask`, which gives a higher score for pixels that more likely correspond to points on the mustard bottle.\n", "\n", "Note that we defined `mustard_ycb_idx = 3` at the top of the notebook; that's the value of the label for the class we care about. Additionally, note that the lables are ordered according to their score, and since np.argmax picks the first element if there is a tie, the best fit is chosen.\n",
i'll take it.
|
I've incorporated all the feedback. thanks! |
This pull request is of commits with code changes that seem less trivial and might need a bit more review. I found the changes helped me get through the exercises, but it is possible that the changes are more "changing the solution to match my result" rather than "correction to the solution". Each commit gives a bit more explanation of what I thought the issue was.
This change is