{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2846","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2846/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2846/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2846/events","html_url":"https://github.com/RobotLocomotion/drake/pull/2846","id":166448337,"number":2846,"title":"Initial implementation of Diagram in System2.","user":{"login":"david-german-tri","id":17437069,"avatar_url":"https://avatars.githubusercontent.com/u/17437069?v=3","gravatar_id":"","url":"https://api.github.com/users/david-german-tri","html_url":"https://github.com/david-german-tri","followers_url":"https://api.github.com/users/david-german-tri/followers","following_url":"https://api.github.com/users/david-german-tri/following{/other_user}","gists_url":"https://api.github.com/users/david-german-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/david-german-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/david-german-tri/subscriptions","organizations_url":"https://api.github.com/users/david-german-tri/orgs","repos_url":"https://api.github.com/users/david-german-tri/repos","events_url":"https://api.github.com/users/david-german-tri/events{/privacy}","received_events_url":"https://api.github.com/users/david-german-tri/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"sherm1","id":4088016,"avatar_url":"https://avatars.githubusercontent.com/u/4088016?v=3","gravatar_id":"","url":"https://api.github.com/users/sherm1","html_url":"https://github.com/sherm1","followers_url":"https://api.github.com/users/sherm1/followers","following_url":"https://api.github.com/users/sherm1/following{/other_user}","gists_url":"https://api.github.com/users/sherm1/gists{/gist_id}","starred_url":"https://api.github.com/users/sherm1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sherm1/subscriptions","organizations_url":"https://api.github.com/users/sherm1/orgs","repos_url":"https://api.github.com/users/sherm1/repos","events_url":"https://api.github.com/users/sherm1/events{/privacy}","received_events_url":"https://api.github.com/users/sherm1/received_events","type":"User","site_admin":false},"assignees":[{"login":"sherm1","id":4088016,"avatar_url":"https://avatars.githubusercontent.com/u/4088016?v=3","gravatar_id":"","url":"https://api.github.com/users/sherm1","html_url":"https://github.com/sherm1","followers_url":"https://api.github.com/users/sherm1/followers","following_url":"https://api.github.com/users/sherm1/following{/other_user}","gists_url":"https://api.github.com/users/sherm1/gists{/gist_id}","starred_url":"https://api.github.com/users/sherm1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sherm1/subscriptions","organizations_url":"https://api.github.com/users/sherm1/orgs","repos_url":"https://api.github.com/users/sherm1/repos","events_url":"https://api.github.com/users/sherm1/events{/privacy}","received_events_url":"https://api.github.com/users/sherm1/received_events","type":"User","site_admin":false}],"milestone":null,"comments":8,"created_at":"2016-07-19T22:04:43Z","updated_at":"2016-07-20T23:18:25Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/2846","html_url":"https://github.com/RobotLocomotion/drake/pull/2846","diff_url":"https://github.com/RobotLocomotion/drake/pull/2846.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/2846.patch"},"body":"A Diagram is a graph of Systems that is itself a System.\r\n\r\nNote that some elements of `Context` have been lifted up to `AbstractContext`, from which `DiagramContext` also inherits.\r\n\r\n+@sherm1 for feature review.  I'm going to continue working on test coverage but I think it's in good enough shape to start the review.\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/2846)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/234113004","html_url":"https://github.com/RobotLocomotion/drake/pull/2846#issuecomment-234113004","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2846","id":234113004,"user":{"login":"sherm1","id":4088016,"avatar_url":"https://avatars.githubusercontent.com/u/4088016?v=3","gravatar_id":"","url":"https://api.github.com/users/sherm1","html_url":"https://github.com/sherm1","followers_url":"https://api.github.com/users/sherm1/followers","following_url":"https://api.github.com/users/sherm1/following{/other_user}","gists_url":"https://api.github.com/users/sherm1/gists{/gist_id}","starred_url":"https://api.github.com/users/sherm1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sherm1/subscriptions","organizations_url":"https://api.github.com/users/sherm1/orgs","repos_url":"https://api.github.com/users/sherm1/repos","events_url":"https://api.github.com/users/sherm1/events{/privacy}","received_events_url":"https://api.github.com/users/sherm1/received_events","type":"User","site_admin":false},"created_at":"2016-07-20T23:18:25Z","updated_at":"2016-07-20T23:18:25Z","body":"Another architectural discussion point:\n- A System needs to specify its Inputs and Outputs, so that it can be wired up into a Diagram. But Input and Output ports have properties that affect what kind of wiring can be done (type, sampling method, fixed or variable length values). More specifically Output ports have those properties and Input ports specify which ones are acceptable. There are likely other properties also. Currently there is a SystemOutput type that presumably can capture Output properties, but there is no corresponding SystemInput. That is deferred until a Context is created, but the wiring diagram must have already been built by then. That prevents error detection that could be done at `Connect` time.\n\nMy preference would be to specify everything needed for Diagram-building as part of the System objects. That would allow early rejection of attempts to attach vector-valued inputs to abstract-valued outputs. It would also allow for introspection of System- and Diagram-wiring properties with no Context needed.\n\nYou could reasonably counter that there will always be some checks that must be performed at run time during the `Finalize()` call so all these errors will get detected eventually. I would argue that (a) it is better to catch errors as early as possible, and (b) it is more natural to think of a System as having Inputs and Outputs with certain properties than it is to think of a System having Outputs with properties, but a (System,Context) pair being required in order for there to be Inputs with properties. \n\n---\n\nReviewed 4 of 23 files at r1.\nReview status: 13 of 23 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.\n\n---\n\n*[drake/systems/framework/context.h, line 18 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2846#-KN9UDRl2bUzrRSHsHFE:-KN9UDRmHoFQp314xPI4:-802140818) ([raw file](https://github.com/robotlocomotion/drake/blob/75b76bf06a73a6cf57e029ed9962a4976c4b8c71/drake/systems/framework/context.h#L18)):*\n> ```objc\n> /// The Context is a container for all of the data necessary to uniquely\n> /// determine the computations performed by a leaf System. Specifically, a\n> ///  Context contains and owns the State, and also contains (but does not own)\n> ```\n\nextra space @bol\n\n---\n\n*[drake/systems/framework/context.h, line 19 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2846#-KN9UWO_3uAeu-HzNOlI:-KN9UWO_3uAeu-HzNOlJ:-2051012252) ([raw file](https://github.com/robotlocomotion/drake/blob/75b76bf06a73a6cf57e029ed9962a4976c4b8c71/drake/systems/framework/context.h#L19)):*\n> ```objc\n> /// determine the computations performed by a leaf System. Specifically, a\n> ///  Context contains and owns the State, and also contains (but does not own)\n> /// pointers to the Input, as well as the simulation time and the cache.\n> ```\n\n\"pointers to the value sources for Inputs\"\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/2846#-:-KN9XFsCVSVgob7HFFOn:1017172173)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"sherm1"}}