Add SimpleCarState Splitter #511

apojomovsky · 2018-07-19T18:43:10Z

Fixes #504

This PR adds a new drake system that, splits the vectorized SimpleCarState_v into individual SimpleCarState messages, one for each agent.
Since the whole vector of states is evaluated each time one of the individual states is requested, an initial negative performance impact was expected.
Nonetheless, the use of the caching feature found in this experimental branch in drake helps us to reduce the impact to a minimum, which is expected since the successive calls to the EvalInputPort method by each of the different state outputs would not require a whole recalculation of the vector.
As shown in the performance analysis below, the use of caching allows us to tremendously reduce the time to complete an instance of the delphyne-mobil-perf demo with 20 MOBIL cars, although the attention should focus in the relative times:
- With the caching feature disabled (master) we get a negative impact of about 17% in the time to complete the test.
- Whereas with the caching feature enabled this negative impact gets reduced to about 10%.
- In both cases, reducing the publish rate to 10 Hz reduces this impact to about 4% for both cases (maybe we could get better results with longer tests, I can do them if necessary).

hidmic

@apojomovsky Left some comments!

hidmic · 2018-07-20T12:56:25Z

src/backend/automotive_simulator.cc

+
+  for (int i = 0; i < num_agents; ++i) {
+    // Appends an ignition publisher to the vector for each agent.
+    agent_state_vector.push_back(


@apojomovsky why keeping a vector of pointers, as opposed to just using the returned reference on each loop iteration and then dropping it?

true that! done, thanks!

hidmic · 2018-07-20T12:57:53Z

src/translations/simple_car_state_v_splitter.cc

+namespace delphyne {
+
+using std::placeholders::_1;
+using std::placeholders::_2;


@apojomovsky consider placing these using ... statements closer to where they're being used (i.e. outside the loop but within the constructor).

done, thanks!

hidmic · 2018-07-20T12:58:17Z

src/translations/simple_car_state_v_splitter.cc

+using std::placeholders::_2;
+
+SimpleCarState_v_Splitter::SimpleCarState_v_Splitter(int agents_number) {
+  DeclareAbstractInputPort();


@apojomovsky should we validate agents_number > 0?

sounds good. done, thanks!

hidmic · 2018-07-20T12:59:02Z

src/translations/simple_car_state_v_splitter.cc

+
+  for (int i = 0; i < agents_number; ++i) {
+    auto f = std::bind(&SimpleCarState_v_Splitter::DoSplit, this, _1, _2, i);
+    auto d = std::bind(&SimpleCarState_v_Splitter::DoAlloc, this);


@apojomovsky consider taking d outside the loop, we can reuse it for every output port.

@apojomovsky also, consider giving both functors a more descriptive name.

done, thanks!

hidmic · 2018-07-20T13:03:39Z

src/translations/simple_car_state_v_splitter.cc

+
+std::unique_ptr<drake::systems::AbstractValue>
+SimpleCarState_v_Splitter::DoAlloc() {
+  return drake::systems::AbstractValue::Make(ignition::msgs::SimpleCarState{});


@apojomovsky consider having an ignition::msgs::SimpleCarState model_; instead of instantiating a temporary one each time.

done, thanks!

hidmic · 2018-07-20T13:06:47Z

src/translations/simple_car_state_v_splitter.cc

+
+  // Assigns the state returned by the agent_index to the output.
+  auto& valor = output->GetMutableValue<ignition::msgs::SimpleCarState>();
+  if (simple_car_state_v.states_size() >= agent_index) {


@apojomovsky I wonder if we should let this pass by silently, as opposed to blowing up if it doesn't hold. Are we expecting transient variations here?

just added a validation that throws an exception if the condition is not met

hidmic · 2018-07-20T13:07:21Z

src/translations/simple_car_state_v_splitter.h

+               drake::systems::AbstractValue* output, int agent_index) const;
+
+  /// @brief Allocates an abstract value object.
+  std::unique_ptr<drake::systems::AbstractValue> DoAlloc();


@apojomovsky can be const?

done, thanks!

hidmic · 2018-07-20T13:09:45Z

src/translations/simple_car_state_v_splitter.h

+  /// @brief Sets the output value with the SimpleCarState message that's at the
+  /// given index of the SimpleCarState_v.
+  /// @param[in] context The simulation context.
+  /// @param[in] output A pointer to an abstracted SimpleCarState message.


@apojomovsky missing agent_index argument in documentation. Also, a bit of an explanation as to why its signature is different from that of all other output ports' Calc functions is in order.

done, thanks!

hidmic · 2018-07-20T13:10:03Z

tools/continuous_integration/jenkins/setup_early

@@ -19,7 +19,6 @@ awscli
 curl
 git
 libignition-cmake-dev
-libsqlite3-dev


@apojomovsky what happened here?

Hm, a commit from another PR. Just rebased my branch with master so that this doesn't appear anymore here.

hidmic · 2018-07-20T13:13:59Z

src/translations/simple_car_state_v_splitter.h

+
+/// @brief A system that takes a SimpleCarState_V and splits it into separate
+/// SimpleCarState messages.
+class SimpleCarState_v_Splitter : public drake::systems::LeafSystem<double> {


@apojomovsky consider making this a template: double -> T.

It's funny though, we need the scalar for this system to operate on a simulation but it doesn't use it. I wonder if there're examples of this already in Drake.

done, thanks!

apojomovsky · 2018-07-20T20:02:01Z

Thanks for the review @hidmic , I just finished addressing your comments. PTAL.

clalancette

A few mostly minor things here.

clalancette · 2018-07-23T23:24:01Z

src/backend/automotive_simulator.cc

@@ -1,5 +1,7 @@
 // Copyright 2017 Toyota Research Institute

+#include "backend/automotive_simulator.h"
+
 #include <algorithm>
 #include <functional>
 #include <utility>


Nit: #include <string>

done, thanks!

clalancette · 2018-07-23T23:30:46Z

src/backend/automotive_simulator.cc

+          builder_
+              ->template AddSystem<AgentsStatePublisherSystem>(
+                  std::make_unique<AgentsStatePublisherSystem>(
+                      "agent/" + std::to_string(i + 1) + "/state"))


Hm, kind of gross. Internally the agents are 0-based; are we sure we want them 1-based to the external observer?

You're right. This change came from the index number that the PoseBundle to SimpleCarState_V was taking, that was 1-based (my mistake at that point), now changed it there and here to be 0-based again.

clalancette · 2018-07-23T23:32:27Z

src/translations/CMakeLists.txt

@@ -26,6 +27,7 @@ target_link_libraries(translations
    protobuf_messages
    ${drake_LIBRARIES}
    ${IGNITION-COMMON_LIBRARIES}
+    ignition-msgs2::ignition-msgs2


Minor, but we mostly are using ${IGNITION-MSGS_LIBRARIES} in the code (the one exception is https://github.com/ToyotaResearchInstitute/delphyne/blob/master/test/regression/cpp/CMakeLists.txt#L24, which we should probably change to be consistent).

Restored it for now, although we'll be changing to the library::library format after #507

clalancette · 2018-07-23T23:33:18Z

src/translations/simple_car_state_v_splitter.cc

+namespace delphyne {
+
+template <typename T>
+SimpleCarState_v_Splitter<T>::SimpleCarState_v_Splitter(int agents_number) {


Personally I think num_agents describes the situation better (agents_number could possibly be confused for the number of an agent, which this is not).

Totally agree, just did that. thanks!

clalancette · 2018-07-23T23:36:17Z

src/backend/automotive_simulator.cc

+                      splitter->get_input_port(0));
+
+    for (int i = 0; i < num_agents; ++i) {
+      // Appends an ignition publisher to the vector for each agent.


Does this really do an append? It kind of just looks like it Connects the SimpleCarState_v to the Splitter, but I'm not sure. Could you clarify this comment a bit?

Yes, it adds a new system to the builder on each iteration and connects it to the splitter.
That being said, I just replaced the previous comment block with a more cleared explanation.

apojomovsky · 2018-07-26T15:36:37Z

Thanks for the review @clalancette . Just finished addressing your comments. PTAL

hidmic

Couple comments @apojomovsky, but it looks great for the most part! 👍

hidmic · 2018-07-26T17:58:20Z

src/translations/simple_car_state_v_splitter.cc

+  // Assigns the state returned by the agent_index to the output.
+  auto& mutable_state =
+      output->GetMutableValue<ignition::msgs::SimpleCarState>();
+  if (simple_car_state_v.states_size() >= agent_index) {


@apojomovsky shouldn't be > instead of >=?

You're right, thanks! Done.

hidmic · 2018-07-26T17:59:15Z

src/translations/simple_car_state_v_splitter.h

+  ///
+  /// In contrast to typical system's `Calc` methods, the presence of an extra
+  /// argument in this function's signature comes from the need to specify which
+  /// of the vector element must be used to generate the output.


@apojomovsky consider ...of the vector element... -> ...of the vector elements....

Done, thanks!

hidmic · 2018-07-26T17:59:46Z

src/translations/simple_car_state_v_splitter.h

+  /// @brief Allocates an abstract value object.
+  std::unique_ptr<drake::systems::AbstractValue> DoAlloc() const;
+
+  ignition::msgs::SimpleCarState state_;


@apojomovsky missing documentation.

Oops, just added it. thanks!

clalancette

This looks good to me, except for the one point about > vs >= that @hidmic brought up. Once that is fixed/clarified, I'm happy to approve.

apojomovsky · 2018-07-26T19:06:26Z

An updated table including the results of testing the splitter with subscribers (in this case, by enabling the GUI). Based on these results, the impact of adding a splitter with reduced update rate of 10Hz has a negative performance hit of about 2%

apojomovsky · 2018-07-26T19:08:02Z

Should we leave the publish rate in default or maybe reduce it to 10Hz or so? @clalancette @hidmic

clalancette · 2018-07-26T19:14:55Z

Should we leave the publish rate in default or maybe reduce it to 10Hz or so? @clalancette @hidmic

My opinion is that we should merge this as-is, and then open a separate issue/PR to address the publishing rate (as it may have knock-on effects, I'm not sure). But I could be convinced otherwise.

hidmic · 2018-07-26T19:33:45Z

Agree with @clalancette. This one is ready to go.

apojomovsky · 2018-07-26T19:37:58Z

Thanks guys! Merging...

apojomovsky requested review from clalancette, basicNew and hidmic July 19, 2018 18:43

hidmic suggested changes Jul 20, 2018

View reviewed changes

Added the SimpleCarState_v Splitter.

e748ac8

apojomovsky force-pushed the apojomovsky/simple_car_state_splitter branch 4 times, most recently from 0ec6745 to 8fc2127 Compare July 20, 2018 20:00

apojomovsky force-pushed the apojomovsky/simple_car_state_splitter branch from 8fc2127 to 3d58710 Compare July 20, 2018 20:12

Addressed comments from Michel

04eaf31

apojomovsky force-pushed the apojomovsky/simple_car_state_splitter branch from 3d58710 to 04eaf31 Compare July 20, 2018 20:23

clalancette reviewed Jul 23, 2018

View reviewed changes

Addressed comments from Chris

95044f5

hidmic suggested changes Jul 26, 2018

View reviewed changes

clalancette reviewed Jul 26, 2018

View reviewed changes

Addressed comments

50f0904

clalancette approved these changes Jul 26, 2018

View reviewed changes

hidmic approved these changes Jul 26, 2018

View reviewed changes

apojomovsky merged commit e189962 into master Jul 26, 2018

apojomovsky deleted the apojomovsky/simple_car_state_splitter branch July 26, 2018 19:38

stonier mentioned this pull request Sep 18, 2018

Architectural Improvements & Bugfixes #453

Closed

14 tasks

Add SimpleCarState Splitter #511

Add SimpleCarState Splitter #511

Conversation

apojomovsky commented Jul 19, 2018

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apojomovsky commented Jul 20, 2018

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apojomovsky commented Jul 26, 2018

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

apojomovsky commented Jul 26, 2018

apojomovsky commented Jul 26, 2018

clalancette commented Jul 26, 2018

hidmic commented Jul 26, 2018

apojomovsky commented Jul 26, 2018