Class-based loadable agent params #410

basicNew · 2018-05-15T14:52:33Z

Removes the generic dictionary + linb-any to pass parameters to agents and replaces them with agent-specific classes for params.

Currently WIP.

basicNew · 2018-05-15T15:18:50Z

@hidmic for a first pass.

hidmic

@basicNew I left some comments. Most of the new classes seem to be lacking documentation, consider adding some.

hidmic · 2018-05-15T16:18:23Z

backend/automotive_simulator.cc

+    const drake::maliput::api::RoadGeometry* road) {
+  return AddLoadableAgent(plugin_library_name, "", agent_name,
+                          std::move(initial_state), road,
+                          std::move(std::make_unique<AgentPluginParams>()));


@basicNew FYI just so that you know, this move is redundant. But there's no need to change it.

hidmic · 2018-05-15T16:19:20Z

backend/python_bindings.cc

+#include "../include/delphyne/agent_plugin_base.h"
+#include "../src/agents/mobil_car.h"
+#include "../src/agents/rail_car.h"
+#include "../src/agents/trajectory_car.h"


@basicNew hmmm is this the standard now on Delphyne? It looks.. awkward.

We decided to revert this and remove relative imports. Changed.

hidmic · 2018-05-15T16:37:42Z

include/delphyne/agent_plugin_base.h

+  template <typename ConcreteAgentParams>
+  std::unique_ptr<ConcreteAgentParams> downcast_params(
+      std::unique_ptr<AgentPluginParams> base_params) {
+    DELPHYNE_DEMAND(base_params.get() != nullptr);


@basicNew why aborting if parameters are null? Isn't that a possible condition? I don't see anything enforcing otherwise before calling downcast_params(). And even then, maybe an exception is a bit less harsh than an abort?

Added guards, thanks

hidmic · 2018-05-15T16:38:25Z

include/delphyne/agent_plugin_base.h

+
+class AgentPluginParams {
+ public:
+  virtual ~AgentPluginParams() {}


@basicNew consider making the destructor pure virtual so that this class becomes abstract.

https://www.geeksforgeeks.org/pure-virtual-destructor-c/ :)

Yes, that was a suprise for me too. Though you can have = 0 anyways if I follow that blog correctly (so that you enforce the 'abstractness' of the class).

hidmic · 2018-05-15T16:39:37Z

test/regression/cpp/agent_plugin_loader_test.cc

-  ASSERT_EQ(0, agent->Configure("testname", 0, params, builder.get(), nullptr,
-                                nullptr));
+  auto params = std::make_unique<delphyne::AgentPluginParams>();
+  ASSERT_EQ(0, agent->Configure("testname", 0, builder.get(), nullptr, nullptr,


@basicNew here and below, consider using kNullNameOfTheThing instead of nullptr directly to make this a bit more readable. Same thing with regard to the zero id.

Done, thanks!

hidmic · 2018-05-15T16:45:48Z

src/agents/rail_car.h

+      : lane_direction_(std::move(lane_direction)),
+        start_params_(std::move(start_params)) {}
+
+  drake::automotive::LaneDirection* get_raw_lane_direction() {


@basicNew may be const, both method and returned pointer (I think).

hidmic · 2018-05-15T16:45:56Z

src/agents/rail_car.h

+    return lane_direction_.get();
+  }
+
+  drake::automotive::MaliputRailcarParams<double>* get_raw_start_params() {


@basicNew may be const, both method and returned pointer (I think).

hidmic · 2018-05-15T16:47:59Z

src/agents/trajectory_car.h

+      std::unique_ptr<drake::automotive::Curve2<double>> curve)
+      : curve_(std::move(curve)) {}
+
+  drake::automotive::Curve2<double>* get_raw_curve() { return curve_.get(); }


@basicNew may be const, both method and returned pointer (I think).

hidmic · 2018-05-15T16:48:34Z

src/agents/mobil_car.h

+  bool get_initial_with_s() { return initial_with_s_; }
+
+ private:
+  bool initial_with_s_{true};


@basicNew I'd remove the in-member initialization of initial_with_s_ and delete the default constructor (as you have no way of setting parameters after construction, I presume you've made them immutable on purpose). The same goes for its derived classes.

Forget about above's comment. No default constructor will be implicitly added as there's a constructor defined for the class a few lines above. Had a brain SIGSEGV, sorry.

No worries, thanks for raising it anyway!

hidmic · 2018-05-15T16:58:41Z

include/delphyne/agent_plugin_base.h

+  /// Throws an exception if the provided parameter is not of the expected
+  /// class
+  template <typename ConcreteAgentParams>
+  std::unique_ptr<ConcreteAgentParams> downcast_params(


@basicNew I wonder if this should be part of AgentPluginParams, with an additional check on ConcreteAgentParams, like:

template<typename ConcreteAgentPluginParams, typename std::enable_if<std::is_base_of<AgentPluginParams, ConcreteAgentPluginParams>::value>::type> std::unique_ptr<ConcreteAgentPluginParams> downcast_params(std::unique_ptr<AgentPluginParams> base_params) {...}

+100, like it!

stonier · 2018-05-16T01:21:25Z

backend/automotive_simulator.cc

+    std::unique_ptr<AgentPluginParams> parameters) {
+  return AddLoadableAgent(plugin_library_name, "", agent_name,
+                          std::move(initial_state), road,
+                          std::move(parameters));


@basicNew how do you feel about dropping this method and coercing the user to always specify both plugin_library_name and plugin_name?

It is actually quite handy to have this; as a matter of fact, we are not using the "full" version of it anywhere :). I would vote for leaving it.

stonier · 2018-05-16T01:22:30Z

backend/automotive_simulator.h

@@ -24,6 +24,7 @@
 #include "drake/automotive/simple_car.h"
 #include "drake/automotive/trajectory_car.h"
 #include "drake/common/drake_copyable.h"
+#include "drake/lcm/drake_lcm_interface.h"


Should be able to drop this - went away in #408

+1, rebase leftovers.

stonier · 2018-05-16T01:25:54Z

backend/python_bindings.cc

-  py::class_<linb::any>(m, "Any")
-      .def(py::init<bool&&>())
-      // Keep alive, ownership: `self` keeps `RoadGeometry` alive.
-      .def(py::init<const RoadGeometry*&&>(), py::keep_alive<1, 2>())


Even if we go in this direction, you'll probably want to keep anything maliput related (road related) around so that the user can dig into the roads (i.e. world ground truth) in scenario setup, or callback scripts.

I'm not sure I follow; is this related to the python bindings discussion we were having on slack or is it a different thing?

I was working under the mistaken assumption that the road was instantiated in python. From a look at the PR though, looks like it's getting built by RoadBuilder, which means being instantiated in c++ with ownership transferred to the simulator. This is fine.

Ah, got it.

basicNew · 2018-05-16T20:25:46Z

@hidmic thanks a lot for the detailed review. Ready for another pass when you get a chance.

hidmic

@basicNew A few more comments, it's looking great!

hidmic · 2018-05-17T13:30:48Z

backend/automotive_simulator.cc

@@ -161,6 +161,8 @@ int AutomotiveSimulator<T>::AddLoadableAgent(
  if (plugin_name.empty()) {
    agent = delphyne::LoadPlugin<T>(plugin_library_name);
  } else {
+    std::cerr << "Plgin with Name!!!!!" << std::endl;
+    throw std::runtime_error("Plgin with Name!!!!!");


@basicNew I like the emphasized panicking, but a few considerations:

Shouldn't it be Plgin -> Plugin?

Since you're throwing, code afterwards in the else clause is dead code.

I'd rather not write stdout/stderr from within libraries (I personally hate when libraries do that and I have no control over what my application is going to output).

According to @basicNew, this is a testing leftover. Removing.

hidmic · 2018-05-17T13:32:03Z

backend/automotive_simulator.h

@@ -98,6 +95,8 @@ class AutomotiveSimulator {
      const drake::maliput::api::RoadGeometry* road,
      std::unique_ptr<AgentPluginParams> parameters);

+  /// An handy overload for the case that the plugin doesn't require any extra


@basicNew Consider An handy overload for the case that the plugin... -> A handy overload in case the plugin....

hidmic · 2018-05-17T13:34:46Z

include/delphyne/agent_plugin_base.h

+
+class AgentPluginParams {
+ public:
+  virtual ~AgentPluginParams() {}


Yes, that was a suprise for me too. Though you can have = 0 anyways if I follow that blog correctly (so that you enforce the 'abstractness' of the class).

hidmic · 2018-05-17T13:35:40Z

include/delphyne/agent_plugin_base.h

+            typename std::enable_if<std::is_base_of<
+                AgentPluginParams, ConcreteAgentPluginParams>::value>::type* =
+                nullptr>
+  std::unique_ptr<ConcreteAgentPluginParams> downcast_params(


@basicNew +10, though I'd make it static or move it outside the class (class is just scoping here, there's no access to the current instance).

hidmic · 2018-05-17T13:40:49Z

src/agents/rail_car.cc

      ignerr << "RailCar::Configure(): "
                "The provided initial lane is not within this simulation's "
                "RoadGeometry."
             << std::endl;
      return -1;
    }

+    if (params_->get_raw_start_params() == nullptr) {


@basicNew I can't seem to find where params_->get_raw_start_params() is used besides here.

Nevermind, it's used near the bottom of the method definition.

hidmic · 2018-05-17T13:45:20Z

test/regression/cpp/agent_plugin_loader_test.cc

-  ASSERT_EQ(0, agent->Configure("testname", 0, builder.get(), nullptr, nullptr,
-                                nullptr, std::move(params)));
+  ASSERT_EQ(
+      0, agent->Configure("testname", 0, builder.get(), kNullPoseAggregator,


@basicNew here and below, those 0s could be named too e.g. kSuccessCode and kDefaultID respectively.

hidmic · 2018-05-17T13:47:11Z

test/regression/cpp/agent_plugin_loader_test.cc


 TEST(AgentPluginLoader, Invalid) {
  auto agent = delphyne::LoadPlugin<double>("foo");
  ASSERT_EQ(nullptr, agent);
 }

 static const char* env = "DELPHYNE_AGENT_PLUGIN_PATH=agent_plugin";
+static const drake::maliput::api::RoadGeometry* kNullRoad{nullptr};

 TEST(AgentPluginLoader, SimpleAgent) {


@basicNew I've just noticed that all three tests are identical except for a type. If there're no plans for this to be different, consider using typed tests.

hidmic · 2018-05-17T13:48:09Z

test/regression/cpp/agent_plugin_loader_test.cc


 TEST(AgentPluginLoader, Invalid) {
  auto agent = delphyne::LoadPlugin<double>("foo");
  ASSERT_EQ(nullptr, agent);
 }

 static const char* env = "DELPHYNE_AGENT_PLUGIN_PATH=agent_plugin";
+static const drake::maliput::api::RoadGeometry* kNullRoad{nullptr};


@basicNew IMHO the last two lines should go like static const type* const name.

hidmic · 2018-05-17T13:48:53Z

test/regression/cpp/automotive_simulator_test.cc

-  ASSERT_EQ(-1, simulator->AddLoadableAgent("NonExistentPlugin",
-                                            "my_test_model", nullptr, nullptr));
+  auto state = std::make_unique<SimpleCarState<double>>();
+  ASSERT_EQ(-1,


@basicNew same as above regarding -1 e.g. kErrorCode.

hidmic · 2018-05-22T21:07:44Z

I had to rebase against master, there was a dependency change along the way.

hidmic · 2018-05-22T21:08:25Z

I feel bad approving myself, @basicNew @apojomovsky mind to take a look?

basicNew · 2018-05-23T12:20:25Z

@hidmic thanks for finishing this one! Couple of minor things/checks:

Could you squash the last two commits? They are both "Address comments".
In case you missed any of them, there are tools/reformat_code.sh, tools/run_cpplint.sh and tools/run_python_checks.sh that should be checked.

Aside from that it is good to go!

hidmic · 2018-05-23T14:02:31Z

@basicNew Commits and curated and tools run (I wonder if we should have commit hooks, though I bet that was discussed before). Also, it seems that pylint doesn't play along with the delphyne.bindings module. We might want to fix/workaround that later on.

basicNew · 2018-05-23T14:04:25Z

@hidmic yup, known issue with pylint. Feel free to merge once tests are green. Thanks!

basicNew added the status: do not merge label May 15, 2018

basicNew force-pushed the basicnew/loadable-agent-params branch 2 times, most recently from 0965c87 to 72c2aed Compare May 15, 2018 15:18

basicNew requested a review from hidmic May 15, 2018 15:18

hidmic suggested changes May 15, 2018

View reviewed changes

stonier reviewed May 16, 2018

View reviewed changes

hidmic suggested changes May 17, 2018

View reviewed changes

hidmic force-pushed the basicnew/loadable-agent-params branch from 7e7d5de to 82a64dd Compare May 22, 2018 21:07

hidmic approved these changes May 22, 2018

View reviewed changes

basicNew removed the status: do not merge label May 23, 2018

stonier mentioned this pull request May 23, 2018

Python bound trajectory agent #417

Merged

Replaced linb-any maps with agent-specific classes for agent params.

9bfaa20

hidmic force-pushed the basicnew/loadable-agent-params branch from 82a64dd to 9bfaa20 Compare May 23, 2018 14:00

hidmic merged commit acd4b13 into master May 23, 2018

basicNew mentioned this pull request May 23, 2018

Review lifetime of loadable agent params #383

Closed

stonier deleted the basicnew/loadable-agent-params branch May 24, 2018 02:23

Class-based loadable agent params #410

Class-based loadable agent params #410

Conversation

basicNew commented May 15, 2018

basicNew commented May 15, 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

hidmic May 15, 2018 • edited Loading

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

basicNew commented May 16, 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

hidmic commented May 22, 2018

hidmic commented May 22, 2018

basicNew commented May 23, 2018

hidmic commented May 23, 2018 • edited Loading

basicNew commented May 23, 2018

hidmic May 15, 2018 •

edited

Loading

hidmic commented May 23, 2018 •

edited

Loading