{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/5945","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/5945/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/5945/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/5945/events","html_url":"https://github.com/RobotLocomotion/drake/pull/5945","id":224305343,"number":5945,"title":"Update RotationalInertia class and unit tests","user":{"login":"mitiguy","id":21087628,"avatar_url":"https://avatars1.githubusercontent.com/u/21087628?v=3","gravatar_id":"","url":"https://api.github.com/users/mitiguy","html_url":"https://github.com/mitiguy","followers_url":"https://api.github.com/users/mitiguy/followers","following_url":"https://api.github.com/users/mitiguy/following{/other_user}","gists_url":"https://api.github.com/users/mitiguy/gists{/gist_id}","starred_url":"https://api.github.com/users/mitiguy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mitiguy/subscriptions","organizations_url":"https://api.github.com/users/mitiguy/orgs","repos_url":"https://api.github.com/users/mitiguy/repos","events_url":"https://api.github.com/users/mitiguy/events{/privacy}","received_events_url":"https://api.github.com/users/mitiguy/received_events","type":"User","site_admin":false},"labels":[{"id":470891318,"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/status:%20curate%20commits%20before%20merging","name":"status: curate commits before merging","color":"d93f0b","default":false}],"state":"open","locked":false,"assignee":{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars0.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false},"assignees":[{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars0.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false},{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars0.githubusercontent.com/u/17601461?v=3","gravatar_id":"","url":"https://api.github.com/users/amcastro-tri","html_url":"https://github.com/amcastro-tri","followers_url":"https://api.github.com/users/amcastro-tri/followers","following_url":"https://api.github.com/users/amcastro-tri/following{/other_user}","gists_url":"https://api.github.com/users/amcastro-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/amcastro-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amcastro-tri/subscriptions","organizations_url":"https://api.github.com/users/amcastro-tri/orgs","repos_url":"https://api.github.com/users/amcastro-tri/repos","events_url":"https://api.github.com/users/amcastro-tri/events{/privacy}","received_events_url":"https://api.github.com/users/amcastro-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":26,"created_at":"2017-04-25T23:57:08Z","updated_at":"2017-05-10T17:01:28Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/5945","html_url":"https://github.com/RobotLocomotion/drake/pull/5945","diff_url":"https://github.com/RobotLocomotion/drake/pull/5945.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/5945.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/5945)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/300547212","html_url":"https://github.com/RobotLocomotion/drake/pull/5945#issuecomment-300547212","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/5945","id":300547212,"user":{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars0.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false},"created_at":"2017-05-10T17:01:28Z","updated_at":"2017-05-10T17:01:28Z","body":"\n\n\n\nReview status: 2 of 3 files reviewed at latest revision, 42 unresolved discussions.\n\n---\n\n*[drake/multibody/multibody_tree/rotational_inertia.h, line 160 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/5945#-Kjk5x7d8BPlefOZzSIo:-KjnFsu8cj86oQIHdxNz:b-31qstc) ([raw file](https://github.com/robotlocomotion/drake/blob/4570c539d4da5267421778b25750fa2f769c75ab/drake/multibody/multibody_tree/rotational_inertia.h#L160)):*\n<details><summary><i>Previously, sherm1 (Michael Sherman) wrote…</i></summary><blockquote>\n\nI don't understand your proposal here, @jwnimmer-tri. @amcastro-tri had originally written this in a more \"generic\" form using enums which resulted in a lot of hard-to-reason about code. It is better for the class to pick \"Lower\" and use it exclusively and directly than to abstract it so it could be Upper or Lower. There is no ambiguity -- only the lower triangle is in use everywhere in this class. Can you elaborate on what would be added by checking that repeatedly?\n</blockquote></details>\n\nThe \"lower triangle\" choice is used everywhere, except where it's not.  There is at least one place in this PR (the swap) that gets it wrong (unchanged from the version of master, which also appears to be wrong) -- unless I'm misreading the code (the discussion is still open), but any case at least documentation improvements are needed.\n\nBy doing ` enum { kTriangleMode = Eigen::Lower; }`, we can say `kTriangleMode` everywhere that we have to tell Eigen what do use, and then `static_assert(kTriangleMode == Eigen::Lower);` as documentation in places we deal with index arithmetic manually -- or possibly even abstract that into a helper that can assert-fail if upper-non-diagonal indices are ever used. \n\nUsing a named variable for the mode would not be done with the intent of being more generic or abstract (i.e., allowing a developer to change which mode was used), but rather with the intent of machine-readable documentation for the developer that is easily searched and cross-referenced, and that fails-fast were someone to try to switch it to upper.\n\nOf course, the enum example is only one solution.  Scatting more comments throughout the class to highlight the places that depend on the upper vs lower choice would be another kind of solution to help prevent future edits from making mistakes.\n\nThe defect is that the choices for valid indices and the lower-triangular assumption are not made clear enough to future developers who might maintain this code.  How it gets made more clear is up to the author.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/5945)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"jwnimmer-tri"}}