{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/4257","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/4257/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/4257/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/4257/events","html_url":"https://github.com/RobotLocomotion/drake/pull/4257","id":191750677,"number":4257,"title":"Implements a simple system that logs its inputs to memory ...","user":{"login":"RussTedrake","id":6442292,"avatar_url":"https://avatars.githubusercontent.com/u/6442292?v=3","gravatar_id":"","url":"https://api.github.com/users/RussTedrake","html_url":"https://github.com/RussTedrake","followers_url":"https://api.github.com/users/RussTedrake/followers","following_url":"https://api.github.com/users/RussTedrake/following{/other_user}","gists_url":"https://api.github.com/users/RussTedrake/gists{/gist_id}","starred_url":"https://api.github.com/users/RussTedrake/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RussTedrake/subscriptions","organizations_url":"https://api.github.com/users/RussTedrake/orgs","repos_url":"https://api.github.com/users/RussTedrake/repos","events_url":"https://api.github.com/users/RussTedrake/events{/privacy}","received_events_url":"https://api.github.com/users/RussTedrake/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":6,"created_at":"2016-11-25T18:19:54Z","updated_at":"2016-11-28T02:21:52Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/4257","html_url":"https://github.com/RobotLocomotion/drake/pull/4257","diff_url":"https://github.com/RobotLocomotion/drake/pull/4257.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/4257.patch"},"body":"which can be retreived after a simulation.  Simplest (but incomplete) workaround to #3734.\r\n\r\nAlso fixes a few bugs/style issues in other places, including:\r\n- simulator was not counting publish events\r\n- affine system was allocating input/output ports even if their dimension was zero (thereby requiring users to attach an empty port to use the system)\r\n- eigen_matrix_compare was printing out rows x rows !!\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/4257)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/263169020","html_url":"https://github.com/RobotLocomotion/drake/pull/4257#issuecomment-263169020","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/4257","id":263169020,"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-11-28T02:21:52Z","updated_at":"2016-11-28T02:21:52Z","body":"+@sherm1 for platform review. Some comments below, PTAL.\n\n---\n\nReviewed 5 of 10 files at r1, 2 of 3 files at r2.\nReview status: 9 of 10 files reviewed at latest revision, 15 unresolved discussions.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.cc, line 27 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXchg04O3FDuBces-JA:-KXchg04O3FDuBces-JB:b-5k3pm1) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.cc#L27)):*\n> ```cpp\n> \n>   // If num_samples exceeds the current allocation, then do a conservative\n>   // resize (ouch!).\n> ```\n\nBTW, agreed -- ouch! As written this has `O(n^2)` complexity for\n`n >> batch_allocation_size`. Consider using a strategy like `std::vector` does instead -- roughly, double the buffer size whenever you are forced to reallocate it. That gives an amortized `O(n)` complexity instead. You could use `batch_allocation_size` as the initial allocation (like `std::vector::reserve()`.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.h, line 23 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXcfOJYKDp5wbgHi5ES:-KXcfOJYKDp5wbgHi5ET:bw6c42e) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.h#L23)):*\n> ```objc\n> class SignalLogger : public LeafSystem<T> {\n>  public:\n>   /// @param input_size Dimension of the (single) input port.\n> ```\n\nBTW this method needs a sentence before the first `@param` to serve as a brief.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.h, line 28 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXcfZ42KkI_vqKXDdDh:-KXcfZ42KkI_vqKXDdDi:bcofnqt) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.h#L28)):*\n> ```objc\n>   explicit SignalLogger(int input_size, int batch_allocation_size = 1000);\n> \n>   /// Non-copyable.\n> ```\n\nBTW, I believe this should be a `//` comment.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.h, line 34 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXcfmKbJ3OTxMcvtR2a:-KXcfmKcoyxmeVHSJJ-r:b6il3y9) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.h#L34)):*\n> ```objc\n>   /// No output.\n>   void EvalOutput(const Context<T>& context,\n>                   SystemOutput<T>* output) const override{};\n> ```\n\nBTW, needs a space before `{}` and has a spurious semicolon afterwards.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.h, line 37 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXcgVqS-XhnP1PSy-71:-KXcgVqS-XhnP1PSy-72:bgfnl) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.h#L37)):*\n> ```objc\n> \n>   /// Access the (simulation) time of the logged data.\n>   const Eigen::VectorBlock<VectorX<T>> sample_times() const {\n> ```\n\nAssuming you don't want the sample times mutable, the return type here should be `Eigen::VectorBlock<const VectorX<T>>` (note placement of `const`). As written the `VectorBlock` is const, but still provides mutable access.\n\n---\n\n*[drake/systems/framework/primitives/signal_logger.h, line 42 at r2](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-KXcgxL5Vp56lcaNW0hT:-KXcgxL5Vp56lcaNW0hU:bspj71h) ([raw file](https://github.com/robotlocomotion/drake/blob/00ac34db102095f84d1408793aabd71f46c53744/drake/systems/framework/primitives/signal_logger.h#L42)):*\n> ```objc\n> \n>   /// Access the logged data.\n>   const Eigen::Block<MatrixX<T>, Eigen::Dynamic, Eigen::Dynamic, true> data()\n> ```\n\nSame comment here about `const` placement -- `const MatrixX<T>`.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/4257#-:-KXcj9Mho9r4XVw1Lo6w:b83prsq)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"sherm1"}}