Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FixValue method to InputPort #10759

Merged
merged 1 commit into from Mar 20, 2019

Conversation

sherm1
Copy link
Member

@sherm1 sherm1 commented Feb 26, 2019

Adds an input_port.FixValue(&context, value) method using the new ValueToAbstractValue & ValueToVectorValue policy classes from #10838.

This is a replacement for the uglier context.FixInputValue(port_num, value) method collection which caused @mposa some usability headaches.

Resolves #10692.

Category            added  modified  removed  
----------------------------------------------
code                153    31        9        
comments            77     21        10       
blank               39     0         0        
----------------------------------------------
TOTAL               269    52        19        

This change is Reviewable

@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch 2 times, most recently from 54635c0 to e1b3fd5 Compare February 26, 2019 16:20
@sherm1 sherm1 changed the title [WIP] Add FixValue method to InputPort Add FixValue method to InputPort Feb 27, 2019
@sherm1 sherm1 marked this pull request as ready for review February 27, 2019 00:09
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@jwnimmer-tri for first refusal on feature review

Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@SeanCurtis-TRI for platform review per rotation, please

Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee seancurtis-tri, platform LGTM missing (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

@jwnimmer-tri
Copy link
Collaborator

a discussion (no related file):
I would like to see the pydrake bindings for this demonstrated before we conclude that the API scheme is satisfactory. Either here, or in a WIP PR that builds on this which we can browse concurrently.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to pause the detailed review while we work on the two top-level discussions.

Reviewed 1 of 3 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee seancurtis-tri, platform LGTM missing (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)

a discussion (no related file):
I suspect that the use cases for this sugar "Give me a ValueType and I'll stash it as an AbstractValue somewhere for you" will be frequent within the framework. What do you think about pulling it out into an internal helper struct?

namespace internal {
template <typename ValueType>
struct ValueEraser {
  using T = ...choose based on ValueType...;
  static auto Construct(const ValueType& value) { return Value<T>(value); }
};
}

which also opens the door to using partial template specializations instead of SFINAE. Then FixValue here be a single overload and we don't even need DRAKE_DOXYGEN guards:

template <typename ValueType>
FixedInputPortValue& FixValue(Context<T>* context, const ValueType& value) const {
  DRAKE_DEMAND(context != nullptr);
  context->FixInputPort(get_index(), internal::ValueEraser<ValueType>::Construct(value));
}

(We could imagine in the future making an internal::ValueUneraser too, factored out of PortBase::Eval so that Context and other places could also use it for their getters.)



systems/framework/input_port.h, line 134 at r1 (raw file):

#else
  // The FixValue() methods here all use the ContextBase FixInputPort()
  // method that takes a unique_ptr<AbstractValue>, rather than the many

minor It would be better to call the const AbstractValue& value overload, because with #8904 we are going to kill the unique_ptr version and keep only the const-ref version.

It also obviates a bunch of make_unique or Clone boilerplate std::make_unique<Value<BasicVector<T>>>(vector) vs Value<BasicVector<T>>>(vector) below, etc.


systems/framework/input_port.h, line 141 at r1 (raw file):

  FixedInputPortValue& FixValue(
      Context<T>* context, const Eigen::Ref<const VectorX<T>>& vector) const {
    return context->FixInputPort(

nit META Probably worth sprinkling some null checks on context prior to anywhere we de-reference it ourselves.


systems/framework/input_port.h, line 147 at r1 (raw file):

  // BasicVector must be special-cased because we permit use of a concrete
  // BasicVector subtype that does not have its own Clone() method. That does
  // not meet the requirements of drake::is_cloneable.

This need isn't really BasicVector specific. If the user gives us a const T& argument here, then we should choose T2* = decltype(std::declval<const T>().Clone().release()) as the Value<T2>'s template argument for type erasure, iff T2 exists.

diff --git a/systems/framework/input_port.h b/systems/framework/input_port.h
index 8f66b8b..28f7bfa 100644
--- a/systems/framework/input_port.h
+++ b/systems/framework/input_port.h
@@ -142,15 +142,6 @@ class InputPort final : public InputPortBase {
         get_index(), std::make_unique<Value<BasicVector<T>>>(vector));
   }
 
-  // BasicVector must be special-cased because we permit use of a concrete
-  // BasicVector subtype that does not have its own Clone() method. That does
-  // not meet the requirements of drake::is_cloneable.
-  FixedInputPortValue& FixValue(Context<T>* context,
-                                const BasicVector<T>& vector) const {
-    return context->FixInputPort(
-        get_index(), std::make_unique<Value<BasicVector<T>>>(vector.Clone()));
-  }
-
   // This signature is used for AbstractValue or Value<U> arguments.
   FixedInputPortValue& FixValue(Context<T>* context,
                                 const AbstractValue& abstract_value) const {
@@ -170,22 +161,29 @@ class InputPort final : public InputPortBase {
     return is_eigen_refable_helper<ValueType>(1);  // Any int will do here.
   }
 
-  // This method won't instantiate if any of the non-templatized signatures
-  // above can be used (after possible conversions). If we allowed this to
-  // instantiate it would be chosen instead of performing those conversions.
-  template <typename ValueType,
-            typename = std::enable_if_t<
-                !(is_eigen_refable<ValueType>() ||
-                  std::is_base_of<AbstractValue, ValueType>::value ||
-                  std::is_base_of<BasicVector<T>, ValueType>::value)>>
+  // These methods won't instantiate if any of the non-templatized signatures
+  // above can be used (after possible conversions). If we allowed these to
+  // instantiate they would be chosen instead of performing those conversions.
+  template <
+    typename ValueType,
+    typename = std::enable_if_t<
+      !is_eigen_refable<ValueType>() &&
+      !std::is_base_of<AbstractValue, ValueType>::value &&
+      std::is_copy_constructible<ValueType>::value>>
+  FixedInputPortValue& FixValue(Context<T>* context,
+                                const ValueType& value) const {
+    return FixValue(&*context, Value<ValueType>(value));
+  }
+  template <
+    typename ValueType,
+    typename ValueT = std::remove_pointer_t<
+      decltype(std::declval<const ValueType>().Clone().release())>,
+    typename = std::enable_if_t<
+      !is_eigen_refable<ValueType>() &&
+      !std::is_base_of<AbstractValue, ValueType>::value>>
   FixedInputPortValue& FixValue(Context<T>* context,
                                 const ValueType& value) const {
-    static_assert(
-        std::is_copy_constructible<ValueType>::value ||
-            drake::is_cloneable<ValueType>::value,
-        "FixValue(): an input port value type must be copy constructible or "
-        "have a public Clone() method.");
-    return FixValueHelper(&*context, value, 1, 1);
+    return FixValue(&*context, Value<ValueT>(value));
   }
 #endif
 
@@ -248,38 +246,6 @@ class InputPort final : public InputPortBase {
     return false;
   }
 
-  // This is instantiated if ValueType has a copy constructor. If it is also
-  // cloneable, this method still gets invoked; we prefer use of the the copy
-  // constructor.
-  template <
-      typename ValueType,
-      typename = std::enable_if_t<std::is_copy_constructible<ValueType>::value>>
-  FixedInputPortValue& FixValueHelper(Context<T>* context,
-                                      const ValueType& value, int, int) const {
-    return context->FixInputPort(get_index(),
-                                 std::make_unique<Value<ValueType>>(value));
-  }
-
-  // This is available if ValueType is cloneable, but is dispreferred if there
-  // is also a copy constructor because the `int, int` args above are a better
-  // match than the `int, ...` args here.
-  template <typename ValueType,
-            typename = std::enable_if_t<drake::is_cloneable<ValueType>::value>>
-  FixedInputPortValue& FixValueHelper(Context<T>* context,
-                                      const ValueType& value, int, ...) const {
-    return context->FixInputPort(
-        get_index(), std::make_unique<Value<ValueType>>(value.Clone()));
-  }
-
-  // This signature exists for non-copyable, non-cloneable ValueType just to
-  // avoid spurious compilation errors. A static_assert above will have provided
-  // a good message already; this method just needs to compile peacefully.
-  template <typename ValueType>
-  FixedInputPortValue& FixValueHelper(Context<T>*, const ValueType&,
-                                      ...) const {
-    DRAKE_UNREACHABLE();
-  }
-
   const System<T>& system_;
 };
 

Note that my patch makes a unit test bark. But if you follow the TODO to MyVector from test_utilities it says "This is extremely dangerous" so yeah. We should stop doing that (or change the test case to expect Clone to be obeyed -- expect Value<MyVector> instead of Value<BasicVector>.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r1.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee seancurtis-tri, platform LGTM missing (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I suspect that the use cases for this sugar "Give me a ValueType and I'll stash it as an AbstractValue somewhere for you" will be frequent within the framework. What do you think about pulling it out into an internal helper struct?

namespace internal {
template <typename ValueType>
struct ValueEraser {
  using T = ...choose based on ValueType...;
  static auto Construct(const ValueType& value) { return Value<T>(value); }
};
}

which also opens the door to using partial template specializations instead of SFINAE. Then FixValue here be a single overload and we don't even need DRAKE_DOXYGEN guards:

template <typename ValueType>
FixedInputPortValue& FixValue(Context<T>* context, const ValueType& value) const {
  DRAKE_DEMAND(context != nullptr);
  context->FixInputPort(get_index(), internal::ValueEraser<ValueType>::Construct(value));
}

(We could imagine in the future making an internal::ValueUneraser too, factored out of PortBase::Eval so that Context and other places could also use it for their getters.)

To be clear, I'm fine either way, but it seems perhaps easier to test, maintain, and extend if the Eraser concept were carved out. We can refactor to that later though.



systems/framework/test/input_port_test.cc, line 162 at r1 (raw file):

            DeclareAbstractInputPort("double_port", Value<double>(1.25))},
        string_port{DeclareAbstractInputPort("string_port",
                                             Value<std::string>("hello"))} {}

For completeness, I would like to see us test an abstract-valued port that uses Values that clone instead of copy.


systems/framework/test/input_port_test.cc, line 198 at r1 (raw file):

      expected_vec);
  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.basic_vec_port.Eval<Eigen::Vector3d>(*context), std::logic_error,

nit Actually, I almost wish this Eval format did work. Consider using Eval<std::string> or something here, if you just want to test a more clearly inappropriate user request, or using port.GetNiceTypeName() if you only wanted to confirm the ValueType that was used for erasing.


systems/framework/test/input_port_test.cc, line 222 at r1 (raw file):

  const BasicVector<double> basic_vector3({7., 8., 9.});
  dut.basic_vec_port.FixValue(&*context, basic_vector3);  // (2)
  dut.derived_vec_port.FixValue(&*context, basic_vector3);

This is not an allowed operation -- you can't fix a BasicVector when the runtime type is supposed to be MyVector3 -- the System's Calc methods would try to downcast it to MyVector3 and fail at runtime.

I haven't tried to figure out whether the lack of exception here is just because we've carved too narrow the code under test, or whether there is real bug here. Maybe only because #9669 isn't done yet?

In any case, it's not okay to have this statement in a unit test presented in a way that makes it look like sunny-day test case. At minimum, there should be some cautionary comment.


systems/framework/test/input_port_test.cc, line 231 at r1 (raw file):

  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.derived_vec_port.FixValue(&*context, basic_vector2), std::logic_error,
      ".*expected.*size=3.*actual.*size=2.*");

nit Given my post a few above on dut.derived_vec_port.FixValue(&*context, basic_vector3);, I am not sure this check makes sense anymore.


systems/framework/test/input_port_test.cc, line 269 at r1 (raw file):

  // Test API (3) that takes an AbstractValue directly.

  // We should only accept the right kind of abstract value.

nit Consider something more like

const Value<int> some_int_value(42);
const AbstractValue& some_int_abstract_value = some_int_value;
// FixValue(context, some_int_value);
// FixValue(context, some_int_abstract_value);

... so that we very clearly cover both const Value<T>& and const AbstractValue& argument types. (Also because using *AbstractValue::Make<T>(x...) is a bit silly when Value<T>(x...) works and we don't need ownership transfer.)

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to stall on platform review until feature review is done, unless you want me to join in on the conversation.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee seancurtis-tri, platform LGTM missing (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-@SeanCurtis-TRI I'll unplug you from this Sean and reassign platform review later.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri and @sherm1)

@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch 3 times, most recently from 98ee3af to deda9e2 Compare March 6, 2019 04:49
@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch 2 times, most recently from 3301a73 to 29ba511 Compare March 7, 2019 01:27
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most review comments addressed, ptal. Remaining:

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri and @sherm1)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To be clear, I'm fine either way, but it seems perhaps easier to test, maintain, and extend if the Eraser concept were carved out. We can refactor to that later though.

Done.



systems/framework/input_port.h, line 134 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

minor It would be better to call the const AbstractValue& value overload, because with #8904 we are going to kill the unique_ptr version and keep only the const-ref version.

It also obviates a bunch of make_unique or Clone boilerplate std::make_unique<Value<BasicVector<T>>>(vector) vs Value<BasicVector<T>>>(vector) below, etc.

Done.


systems/framework/input_port.h, line 141 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit META Probably worth sprinkling some null checks on context prior to anywhere we de-reference it ourselves.

Done.


systems/framework/input_port.h, line 147 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This need isn't really BasicVector specific. If the user gives us a const T& argument here, then we should choose T2* = decltype(std::declval<const T>().Clone().release()) as the Value<T2>'s template argument for type erasure, iff T2 exists.

diff --git a/systems/framework/input_port.h b/systems/framework/input_port.h
index 8f66b8b..28f7bfa 100644
--- a/systems/framework/input_port.h
+++ b/systems/framework/input_port.h
@@ -142,15 +142,6 @@ class InputPort final : public InputPortBase {
         get_index(), std::make_unique<Value<BasicVector<T>>>(vector));
   }
 
-  // BasicVector must be special-cased because we permit use of a concrete
-  // BasicVector subtype that does not have its own Clone() method. That does
-  // not meet the requirements of drake::is_cloneable.
-  FixedInputPortValue& FixValue(Context<T>* context,
-                                const BasicVector<T>& vector) const {
-    return context->FixInputPort(
-        get_index(), std::make_unique<Value<BasicVector<T>>>(vector.Clone()));
-  }
-
   // This signature is used for AbstractValue or Value<U> arguments.
   FixedInputPortValue& FixValue(Context<T>* context,
                                 const AbstractValue& abstract_value) const {
@@ -170,22 +161,29 @@ class InputPort final : public InputPortBase {
     return is_eigen_refable_helper<ValueType>(1);  // Any int will do here.
   }
 
-  // This method won't instantiate if any of the non-templatized signatures
-  // above can be used (after possible conversions). If we allowed this to
-  // instantiate it would be chosen instead of performing those conversions.
-  template <typename ValueType,
-            typename = std::enable_if_t<
-                !(is_eigen_refable<ValueType>() ||
-                  std::is_base_of<AbstractValue, ValueType>::value ||
-                  std::is_base_of<BasicVector<T>, ValueType>::value)>>
+  // These methods won't instantiate if any of the non-templatized signatures
+  // above can be used (after possible conversions). If we allowed these to
+  // instantiate they would be chosen instead of performing those conversions.
+  template <
+    typename ValueType,
+    typename = std::enable_if_t<
+      !is_eigen_refable<ValueType>() &&
+      !std::is_base_of<AbstractValue, ValueType>::value &&
+      std::is_copy_constructible<ValueType>::value>>
+  FixedInputPortValue& FixValue(Context<T>* context,
+                                const ValueType& value) const {
+    return FixValue(&*context, Value<ValueType>(value));
+  }
+  template <
+    typename ValueType,
+    typename ValueT = std::remove_pointer_t<
+      decltype(std::declval<const ValueType>().Clone().release())>,
+    typename = std::enable_if_t<
+      !is_eigen_refable<ValueType>() &&
+      !std::is_base_of<AbstractValue, ValueType>::value>>
   FixedInputPortValue& FixValue(Context<T>* context,
                                 const ValueType& value) const {
-    static_assert(
-        std::is_copy_constructible<ValueType>::value ||
-            drake::is_cloneable<ValueType>::value,
-        "FixValue(): an input port value type must be copy constructible or "
-        "have a public Clone() method.");
-    return FixValueHelper(&*context, value, 1, 1);
+    return FixValue(&*context, Value<ValueT>(value));
   }
 #endif
 
@@ -248,38 +246,6 @@ class InputPort final : public InputPortBase {
     return false;
   }
 
-  // This is instantiated if ValueType has a copy constructor. If it is also
-  // cloneable, this method still gets invoked; we prefer use of the the copy
-  // constructor.
-  template <
-      typename ValueType,
-      typename = std::enable_if_t<std::is_copy_constructible<ValueType>::value>>
-  FixedInputPortValue& FixValueHelper(Context<T>* context,
-                                      const ValueType& value, int, int) const {
-    return context->FixInputPort(get_index(),
-                                 std::make_unique<Value<ValueType>>(value));
-  }
-
-  // This is available if ValueType is cloneable, but is dispreferred if there
-  // is also a copy constructor because the `int, int` args above are a better
-  // match than the `int, ...` args here.
-  template <typename ValueType,
-            typename = std::enable_if_t<drake::is_cloneable<ValueType>::value>>
-  FixedInputPortValue& FixValueHelper(Context<T>* context,
-                                      const ValueType& value, int, ...) const {
-    return context->FixInputPort(
-        get_index(), std::make_unique<Value<ValueType>>(value.Clone()));
-  }
-
-  // This signature exists for non-copyable, non-cloneable ValueType just to
-  // avoid spurious compilation errors. A static_assert above will have provided
-  // a good message already; this method just needs to compile peacefully.
-  template <typename ValueType>
-  FixedInputPortValue& FixValueHelper(Context<T>*, const ValueType&,
-                                      ...) const {
-    DRAKE_UNREACHABLE();
-  }
-
   const System<T>& system_;
 };
 

Note that my patch makes a unit test bark. But if you follow the TODO to MyVector from test_utilities it says "This is extremely dangerous" so yeah. We should stop doing that (or change the test case to expect Clone to be obeyed -- expect Value<MyVector> instead of Value<BasicVector>.

Done.


systems/framework/test/input_port_test.cc, line 162 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For completeness, I would like to see us test an abstract-valued port that uses Values that clone instead of copy.

OK, unit tests for ValueToAbstractValue now cover all the cases. This isn't handled by code in InputPort now.


systems/framework/test/input_port_test.cc, line 198 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Actually, I almost wish this Eval format did work. Consider using Eval<std::string> or something here, if you just want to test a more clearly inappropriate user request, or using port.GetNiceTypeName() if you only wanted to confirm the ValueType that was used for erasing.

Done. I added a more straightforward test but left this one in with a TODO. I agree it would be very nice to have this work since users will expect it to -- should be feasible with some more specialization, I think.


systems/framework/test/input_port_test.cc, line 222 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is not an allowed operation -- you can't fix a BasicVector when the runtime type is supposed to be MyVector3 -- the System's Calc methods would try to downcast it to MyVector3 and fail at runtime.

I haven't tried to figure out whether the lack of exception here is just because we've carved too narrow the code under test, or whether there is real bug here. Maybe only because #9669 isn't done yet?

In any case, it's not okay to have this statement in a unit test presented in a way that makes it look like sunny-day test case. At minimum, there should be some cautionary comment.

Added a TODO for now but I'm going to dig into this a bit more to see if I can't get it to cough up a good error message.


systems/framework/test/input_port_test.cc, line 231 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Given my post a few above on dut.derived_vec_port.FixValue(&*context, basic_vector3);, I am not sure this check makes sense anymore.

Done. Switched to a type that should work.


systems/framework/test/input_port_test.cc, line 269 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Consider something more like

const Value<int> some_int_value(42);
const AbstractValue& some_int_abstract_value = some_int_value;
// FixValue(context, some_int_value);
// FixValue(context, some_int_abstract_value);

... so that we very clearly cover both const Value<T>& and const AbstractValue& argument types. (Also because using *AbstractValue::Make<T>(x...) is a bit silly when Value<T>(x...) works and we don't need ownership transfer.)

Done.

@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch from 29ba511 to 37b8e74 Compare March 7, 2019 02:02
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri and @sherm1)


systems/framework/test/input_port_test.cc, line 222 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Added a TODO for now but I'm going to dig into this a bit more to see if I can't get it to cough up a good error message.

OK, I see there is already a TODO here to add this check, and an issue #9669. A possible approach would be to add a method like
template <typename Candidate> bool AbstractValue::CanHoldCandidate() which would punt to Value<T> where we could verify that T is polymorphic and that std::is_base_of<T, Candidate>.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 6 files at r2.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri and @sherm1)


systems/framework/input_port.h, line 3 at r2 (raw file):

#pragma once

#include <memory>

nit Unused (by the current patch below).


systems/framework/input_port.h, line 110 at r2 (raw file):

  supplied and its concrete type will be preserved. Any other ValueType will
  be converted to an AbstractValue whose underlying type is `Value<ValueType>`
  and this must be an abstract port.

nit Saying "whose underlying type is Value<ValueType>" is not precisely correct, when there is a base class involved.

@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch from 37b8e74 to 53b73f4 Compare March 8, 2019 00:54
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwnimmer-tri, I think Python wrapping was the only remaining feature review sticking point? I'm not going to do that in this PR and I don't think it makes sense to keep this in stasis while we wait.
+@EricCousineau-TRI for platform review per rotation, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @sherm1)

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Mar 15, 2019

I agree that it's good to start platform review now.

In the past of systems framework, we've had to throw away and rewrite our AbstractValue-related APIs once we tried to add pydrake to them and found them impossible to manage. I think we need to retire that risk here, before we merge this PR and ask people to start using it. I think Eric (or I) should be able to prototype that relatively quickly.

@sherm1
Copy link
Member Author

sherm1 commented Mar 15, 2019

OK.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@EricCousineau-TRI I'd to see a least a proof-of-concept that pydrake APIs make sense, before we commit the C++ APIs to master, so that we don't have to come back and rework them later. Could you open a draft PR (atop this one) with a proposal?

Per chat with Eric last week, I'm going to take a spin at the bindings. WIP so far is https://github.com/jwnimmer-tri/drake/tree/add_fixvalue_to_input_port.

The VectorPolicy overloads seem to have worked out fine. I am still struggling with the AbstractPolicy overloads. (See below for one particularly hairy clause.)


a discussion (no related file):
@sherm1 For the AbstractPolicy clause (3):

3. Any Eigen expression type is simplified using `.eval()`, and then stored
in a Value<E> where E is the type returned by `.eval()` (without const).

... do we have any use case identified?

So far, it seems quite difficult to make this work in pydrake. Even for C++, I'd argue that it's too obscure to be valuable. If the user wants a type-erase particular Eigen Matrix size Value<Eigen::Matrix<double, 3, 4>> or whatever, I think we should make them write out the typename, instead of inferring it from the Eigen expression tree. As shown by the policy's unit test (where decltype had to be used), it's too difficult for the user to predict what will be erased:

using Tail3StorageType = std::remove_const_t<
std::remove_reference_t<decltype(long_vec.tail(3).eval())>>;

... and as soon as they put int Matrix34 and try to pull out MatrixXd, they will be surprised and sad.

How about we remove this specialization, and/or turn it into an exception?


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @sherm1)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Per chat with Eric last week, I'm going to take a spin at the bindings. WIP so far is https://github.com/jwnimmer-tri/drake/tree/add_fixvalue_to_input_port.

The VectorPolicy overloads seem to have worked out fine. I am still struggling with the AbstractPolicy overloads. (See below for one particularly hairy clause.)

I'm resolving this top-level discussion asking for a pydrake proof of concept. I've written that up in #10945 now. The only open issue is the below discussion, where the AbstractValue Eigen magic is too magical.


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@sherm1 For the AbstractPolicy clause (3):

3. Any Eigen expression type is simplified using `.eval()`, and then stored
in a Value<E> where E is the type returned by `.eval()` (without const).

... do we have any use case identified?

So far, it seems quite difficult to make this work in pydrake. Even for C++, I'd argue that it's too obscure to be valuable. If the user wants a type-erase particular Eigen Matrix size Value<Eigen::Matrix<double, 3, 4>> or whatever, I think we should make them write out the typename, instead of inferring it from the Eigen expression tree. As shown by the policy's unit test (where decltype had to be used), it's too difficult for the user to predict what will be erased:

using Tail3StorageType = std::remove_const_t<
std::remove_reference_t<decltype(long_vec.tail(3).eval())>>;

... and as soon as they put int Matrix34 and try to pull out MatrixXd, they will be surprised and sad.

How about we remove this specialization, and/or turn it into an exception?

Yup. After finishing the prototype, I don't see how (3) can survive. It should be a runtime error in C++.


Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yup. After finishing the prototype, I don't see how (3) can survive. It should be a runtime error in C++.

I'm happy to give up the .eval() shenanigans (learned that from Eric :), but I don't see a reason to avoid just-plain-Eigen-objects like Matrix3d's and whatnot. I assume those would map nicely from numpy objects -- I can make signature 3 accept those and give a runtime error for ugly Eigen types. Can you make an analogous Python binding for that?


@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch from 93618bd to dab9dd4 Compare March 19, 2019 01:01
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @jwnimmer-tri)

a discussion (no related file):

Previously, sherm1 (Michael Sherman) wrote…

I'm happy to give up the .eval() shenanigans (learned that from Eric :), but I don't see a reason to avoid just-plain-Eigen-objects like Matrix3d's and whatnot. I assume those would map nicely from numpy objects -- I can make signature 3 accept those and give a runtime error for ugly Eigen types. Can you make an analogous Python binding for that?

Done, maybe. I pushed the version of AbstractPolicy where only "nice" Eigen types are acceptable, PTAL.


Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass done; chimed in on fixed-size Eigen matrices tidbit.

Reviewed 1 of 6 files at r3, 2 of 4 files at r6.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

a discussion (no related file):

Previously, sherm1 (Michael Sherman) wrote…

Done, maybe. I pushed the version of AbstractPolicy where only "nice" Eigen types are acceptable, PTAL.

I'm not sure if I want Python type registration to be littered with registration of fixed-sized matrix shenanigans...
(See, a word so precise that it can be polymorphic! :P)

I'd accept it if it were empirically shown to drastically improve performance (or subjectively shown to improve usability) in a non-trivial usage.
Pending that, I'd prefer that we not permit it via conjecture. At present, users can simply wrap their usages of whatever fanciful Eigen type using a struct, and they can explicitly register that.

At some point, I might be fine with MatrixX<T>, and maybe even a fixed subset of matrix types stored (e.g. Matrix3<T>, Matrix3X<T>), but would prefer we punt that to discussion in an issue / future PR.

For now, I'd strongly recommend that this signature only accept Ref<const VectorX<T>> which is stored as BasicVector<T>, and anything else be a run- or (preferably) compile-time error.



systems/framework/value_to_abstract_value.h, line 72 at r6 (raw file):

 1. Any given AbstractValue object is simply cloned.
 2. A `char *` type is copied into a Value<std::string>.
 3. Simple Eigen objects (e.g. Matrix3d) are accepted and stored as themselves;

To crank down on this (per above comment):
I'd really prefer that we don't encourage using fixed-size matrices, and least not yet; can this instead only be VectorX<T>?


systems/framework/value_to_abstract_value.h, line 203 at r6 (raw file):

  // TODO(sherm1) Replace with std::remove_cvref_t for C++20.
  template <typename ValueType>
  using RemoveCVRef = std::remove_cv_t<std::remove_reference_t<ValueType>>;

I believe decay_t would cover your use case?
https://en.cppreference.com/w/cpp/types/decay

(I'm not sure if this should even stay per other discussions; however, if you're doing a compile-time error, this would help!)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'm not sure if I want Python type registration to be littered with registration of fixed-sized matrix shenanigans...
(See, a word so precise that it can be polymorphic! :P)

I'd accept it if it were empirically shown to drastically improve performance (or subjectively shown to improve usability) in a non-trivial usage.
Pending that, I'd prefer that we not permit it via conjecture. At present, users can simply wrap their usages of whatever fanciful Eigen type using a struct, and they can explicitly register that.

At some point, I might be fine with MatrixX<T>, and maybe even a fixed subset of matrix types stored (e.g. Matrix3<T>, Matrix3X<T>), but would prefer we punt that to discussion in an issue / future PR.

For now, I'd strongly recommend that this signature only accept Ref<const VectorX<T>> which is stored as BasicVector<T>, and anything else be a run- or (preferably) compile-time error.

@sherm1 The first part of my question was "do we have any use case identified"? I haven't seen any yet.

Remember that we're not ruling out using Matrix3d as the value type. We're just forcing the user to say port.FixValue(Value<Matrix3d>(mymat)); -- where they have to write out the typename they want to use.

Until we see a few call sites that use such syntax and feel it is too much boilerplate, why should special-case it now? Code in systems/framework that nobody ever calls is just a waste of our time. If you can't find one System in Drake that needs this sugar, maybe it's a bad trade-off to add it now?


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r6.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

a discussion (no related file):

Can you make an analogous Python binding for that?

To answer this directly: for the Fix bindings, we will use whatever type was declared for the port when the port was created. So if the model_value at port-declaration-time was Value<PortT>(), and if Value<PortT>::set_value(foo) works for some foo, then InputPort::FixValue(foo) will work as well. So if it's Value<Matrix3d> then yes, a 3x3 numpy would work in a pydrake call to FixValue.

My objection is not that the pydrake bindings are difficult -- it's that the C++ feature is both dangerous and useless in the first place.


@sherm1
Copy link
Member Author

sherm1 commented Mar 19, 2019

why should we special-case it now?

We have to special-case the abstract_port policy for Eigen objects so that vector_port.FixValue(eigen_expression) (note vector) will compile (the actual problem is that Eigen's secret types T don't meet our requirements for Value<T>). While I'm special-casing I can reject the objects or accept them. If I reject them I feel obligated to generate a helpful error message.
To generate a helpful error message I have to do the same SFINAE shenanigans :) as to accept them. If I understand the current proposal, it means that if someone tries abstract_port.FixValue(MatrixXd) I should issue an error message something like:

MatrixXd is not a permissible type for a Drake abstract object. Use
Value<MatrixXd> instead.

but if they write abstract_port.FixValue(3. * MatrixXd) I can't use the given inscrutable type in the error message; to be helpful it ought to say something like:

The Eigen expression you provided does not have a permissible type
for a Drake abstract object. Use Value<ActualType>(expression) instead.

where I can figure out the ActualType to put in the message (better than the API user).

It is certainly true that Eigen objects can't be put in abstract ports at the moment since context.FixInputPort() doesn't support them, but that can also be said for all the other
concrete types like int and std::string. Literally the whole point of this project was to provide a nicer API for fixing port values. If int and string are included, the argument against MatrixXd seems weak.

The amount of unused code is going to be about the same whether I accept Eigen objects or not, if I provide a helpful error message. Why is it better to reject than accept them?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we need to have AbstractPolicy compile for Eigen types, because VectorPolicy has to do so.

If the error message is "The provided Eigen expression does not type-erase automatically. Use ValueEigen::MatrixXd(expression) or similar instead.", that seems easy and not a lot of code? I reject the premise that we need to deduce the eigen-eval-type for the suggestion. I think we must not recommend one particular type-erased key type, because there are so many to choose from.

Doing string implicitly in this conversion is fine, because there's only one common kind of string. Given that Matrix3d and MatrixXd are both valid erased types (as are VectorXd, Vector3d, ArrayXd, Array3d, VectorUpTo3d, etc.) and a user can trivially assign (implicitly convert) between those types in their own code, it is brittle to assume that the user wanted a fixed vs dynamic-sized matrix as storage, or that they want VectorXd instead of MatrixXd. We should force users to be explicit when erasing Eigen types. Whether that's FixValue<Matrix3d>(mymat) or FixValue(Value<Matrix3d>(mymat)) I don't really care, but FixValue(mymat) is right out.

And again, I think that nobody should care. If someone is placing an Eigen directly into a AbstractValue they're doing it wrong. They should be using a struct MyCacheData { Matrix3<T> stuff; } or whatever, not a bare Eigen. Do we have any plausible user code anywhere that cares whether this overload in AbstractPolicy passes or throws?!

... that can also be said for all the other concrete types like int and std::string ...

I don't understand this. The user can put int and string into a fixed port value, they just have to AbstractValue::Make<int> or whatever, to declare the erase-key type manually, instead of us inferring it. The same already goes for MatrixXd too, right?

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps more directly:

In this PR even without rule (3), the user can place any value they want, using port.FixValue(Value<Foo>(expr)). This PR enables them to do that on the port, instead of the Context - yay!

We have sugar to allow them to say port.FixValue(expr), but it's up to us what we do with various types of expr. I am saying that expr=Eigen is rare for abstract-valued ports, and so does not need sugar. I'm also saying that inferring a type from expr=Eigen is dangerous because Eigen types are so flexible -- it's difficult to know what the user intends.

We could take our best guess and then consult the model_value of the port to see if our guess was correct. That would be OK by me on the "dangerous" front because we're checking against what the user already declared, but on the "rare" front I would still want to see a use case before we invested in that guessing-code.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @sherm1)

@sherm1
Copy link
Member Author

sherm1 commented Mar 19, 2019

OK, thanks for the clear thoughts. I'll attempt a reasonable error message and require Vector<EigenThing>.

@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch from dab9dd4 to f5d6b79 Compare March 19, 2019 22:24
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @jwnimmer-tri)


systems/framework/value_to_abstract_value.h, line 72 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

To crank down on this (per above comment):
I'd really prefer that we don't encourage using fixed-size matrices, and least not yet; can this instead only be VectorX<T>?

Done.


systems/framework/value_to_abstract_value.h, line 203 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I believe decay_t would cover your use case?
https://en.cppreference.com/w/cpp/types/decay

(I'm not sure if this should even stay per other discussions; however, if you're doing a compile-time error, this would help!)

Done (gone now).

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done now, I think -- PTAL.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee ericcousineau-tri, platform LGTM missing (waiting on @EricCousineau-TRI and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Can you make an analogous Python binding for that?

To answer this directly: for the Fix bindings, we will use whatever type was declared for the port when the port was created. So if the model_value at port-declaration-time was Value<PortT>(), and if Value<PortT>::set_value(foo) works for some foo, then InputPort::FixValue(foo) will work as well. So if it's Value<Matrix3d> then yes, a 3x3 numpy would work in a pydrake call to FixValue.

My objection is not that the pydrake bindings are difficult -- it's that the C++ feature is both dangerous and useless in the first place.

Done. Eigen objects and expressions disallowed. I'm issuing a generic message as you suggested so was able to nuke the extra SFINAE muck.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: feature! I will rebase my pydrake branch soonish, so that it's ready to go right after this merges.

Reviewed 2 of 2 files at r7.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee ericcousineau-tri, platform LGTM from [jwnimmer-tri] (waiting on @EricCousineau-TRI)

Revise AbstractPolicy to eliminate sugar for Eigen objects and expressions.
@sherm1 sherm1 force-pushed the add_fixvalue_to_input_port branch from f5d6b79 to c2f1aa4 Compare March 20, 2019 00:29
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r8.
Dismissed @EricCousineau-TRI from 2 discussions.
Reviewable status: all discussions resolved, LGTM missing from assignee ericcousineau-tri, platform LGTM from [jwnimmer-tri]

@jwnimmer-tri
Copy link
Collaborator

@EricCousineau-TRI needs your platform LGTM badge when you're ready.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: - thanks!!!

Reviewed 2 of 6 files at r3, 1 of 2 files at r7, 1 of 1 files at r8.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [jwnimmer-tri, ericcousineau-tri]

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, gents. I'll merge this after CI is green. I'm working on a PR to convert a bunch of exiting context.FixInputPort() calls to port.FixValue() calls to see if this is better in real life.

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [jwnimmer-tri, ericcousineau-tri]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants