{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2970","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2970/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2970/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2970/events","html_url":"https://github.com/RobotLocomotion/drake/pull/2970","id":168172945,"number":2970,"title":"Add a continuous integrator to the System2 primitive repertoire.","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},{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars.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":5,"created_at":"2016-07-28T19:09:32Z","updated_at":"2016-07-29T18:19:54Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/2970","html_url":"https://github.com/RobotLocomotion/drake/pull/2970","diff_url":"https://github.com/RobotLocomotion/drake/pull/2970.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/2970.patch"},"body":"This was a recent topic of conversation, and is useful for unit testing diagram state.  @amcastro-tri and @sherm1 for feature review.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/2970)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/236255433","html_url":"https://github.com/RobotLocomotion/drake/pull/2970#issuecomment-236255433","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/2970","id":236255433,"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},"created_at":"2016-07-29T18:19:54Z","updated_at":"2016-07-29T18:19:54Z","body":"\n\n\n\nReview status: 3 of 7 files reviewed at latest revision, 10 unresolved discussions.\n\n---\n\n*[drake/systems/framework/primitives/integrator.cc, line 20 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNr6SMndC7JRAr2iSId:-KNriHb17iN2OHOW2nIR:1750174961) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.cc#L20)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> I think this method should be provided by the super-class (soon to just be `System<T> per #2872?). That would remove the hassle from the user of having to play around with all sorts of `unique_ptr`'s.\n> I think the constructor I sort of provide above tells the super-class all the information needed to do this.\n> \n</details>\nThank you for this comment, and the one above. I feel like I finally understand what you've been asking for!\n\nHere's the key thing.  This method allows systems to have custom types of input, output, and state vectors.  For instance, in the spring-mass example, the output vector has position and velocity accessors.  Under your proposal, it would only be possible to ask for `[0]` and `[1]`.  \n\nThat said, I agree we could provide a default implementation of `CreateDefaultContext` for systems that don't use fancy types, such as this one. \n\n---\n\n*[drake/systems/framework/primitives/integrator.cc, line 32 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNr6qLypHJcRzGViOc_:-KNrknSzR89HXu-agBjn:160841994) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.cc#L32)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> Same here. I think this could be a method from the super-class. Essentially if you defined all the inputs/outputs in the constructor the super-class has all the information to figure these out. \n</details>\nSame as above.\n\n---\n\n*[drake/systems/framework/primitives/integrator.cc, line 45 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNrBa3nE0xKpjm_wqz7:-KNrkpjmhoiqJJQ7Exjr:1217293170) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.cc#L45)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> And same here. It could be provided by the super-class.\n</details>\nAnd ditto.\n\n---\n\n*[drake/systems/framework/primitives/integrator.cc, line 65 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNrBoTBawjUvZvr6Pa6:-KNrlJFhEkTAOaNL-PRf:702298802) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.cc#L65)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> For earthlings like me I think this could look more like:\n> ```c++\n> VectorX<T> x = get_continuous_state(context);\n> set_output_port(context, get_output_port(0), x);\n> ```\n> where methods like `get_continuous_state`, `set_output_port` and `get_output_port` are provided by `System<T>`. Notice `get_output_port(int port_index)` would return an `OutputPort` (I think this is similar to your PortDescriptor in #2955?).\n> I'll let you figure out the more complex caching problem :-)\n</details>\nYeah, I have no objection to additional syntactic sugar methods.  We have to make sure the names clearly communicate the computational cost, but I agree this code could be made shorter.\n\n(I also don't understand why you'd want to say `get_output_port(0)` instead of just `0`, but that's a detail.)\n\nAlso, this line ww\n\n---\n\n*[drake/systems/framework/primitives/integrator.h, line 32 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNrG4XWFHkTplOoCEJi:-KNrhjXwRdkLDhZa4ky3:-499146009) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.h#L32)):*\n<details><summary><i>Previously, liangfok (Chien-Liang Fok) wrote…</i></summary>\n> `length_` is private. Did you mean \"the length specified as a parameter to the constructor\"?\n</details>\nDone.\n\n---\n\n*[drake/systems/framework/primitives/integrator.h, line 50 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNrEMSiEPmxehXcgVNd:-KNrmBS7kaM8B9SsHD0y:-1078707153) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/integrator.h#L50)):*\n<details><summary><i>Previously, liangfok (Chien-Liang Fok) wrote…</i></summary>\n> That `0` is concerning. Is it guaranteed to always be port zero? Is it a \"magic number\"?\n</details>\n@amcastro-tri Possibly.  I'll defer offering an opinion on this until we see what form of I/O description we adopt.  It's a detail, either way.\n\n@liangfok I think it's intrinsic to the design of this system that there's exactly one input port.  We could make it a named constant for clarity, but it's not really a magic number, no.\n\n---\n\n*[drake/systems/framework/primitives/test/CMakeLists.txt, line 7 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNr3ueYaAnjR9DLZa19:-KNrmoyC8dG1e2IvLbrn:-499146009) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/test/CMakeLists.txt#L7)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> Estrange red symbol in reviewable. I think it means missing new line character?\n</details>\nDone.\n\n---\n\n*[drake/systems/framework/primitives/test/integrator_test.cc, line 56 \\[r1\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/2970#-KNr6D7YSZ1L4SenX44-:-KNrorQe81oWxTyJu6mi:-960440723) ([raw file](https://github.com/robotlocomotion/drake/blob/ee3773116d5ebb744e462fae7b371f3ebba90bf8/drake/systems/framework/primitives/test/integrator_test.cc#L56)):*\n<details><summary><i>Previously, amcastro-tri (Alejandro Castro) wrote…</i></summary>\n> (cont.) ... then the output port data would be in the context right? I still do not understand why we would need to provide the output as an argument to this method. @sherm1 do you have any suggestions on this one?\n</details>\nContext is the const parts, Output is the non-const parts.  I agree we could just lump output into context, but I think the distinction is pretty useful to help system authors not shoot themselves in the foot.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/2970)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"david-german-tri"}}