pydrake systems: Add initial custom scalar conversion test / example

[EricCousineau-TRI]

This shows authoring a scalar-convertible class in Python.
This can be used to author a class, or can be used to write a Python trampoline class, which can then call a simpler Python implementation.

\cc @RussTedrake

---

This change is 

[EricCousineau-TRI]

+@jwnimmer-tri for feature review, please.
+@sherm1 for platform review, please.

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 4 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, all discussions resolved.

---

bindings/pydrake/systems/framework_py_systems.cc, line 416 at r1 (raw file):

        std::function<std::unique_ptr<System<T>>(const System<U>*)>;
    AddTemplateMethod(
        converter, "Add",

Its not immediately obvious to me why the python signature for Add needs to accept the System<U>* as a pointer instead of a reference.

---

bindings/pydrake/systems/scalar_conversion.py, line 47 at r1 (raw file):

    def make_converter(self, param_pairs=None):
        """Creates system scalar converter for a given template, assuming that
        the System class has a scalar-type-converting copy constructing.

Do we expect a pydrake user to call this method directly. If so, I think this doc is quite a bit too obscure and brief. Even as domain expert trying to read the code, I am pretty confused by this whole file.

---

systems/framework/system_scalar_converter.h, line 104 at r1 (raw file):

      : SystemScalarConverter() {
    // N.B. Synchronize these pairs with the `ConversionPairs` type pack in
    // `DefineFrameworkPySystems`.

BTW This phrasing could be slightly improved to more clearly be an instruction to an author changing this list, rather than a reader trying to understand it (i.e., this could be misread as explaining what is happening). Maybe more like "When changing the pairs of supported types below, be sure to also change foo to be the same list of pairs".

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 1 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/systems/framework_py_systems.cc, line 416 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Its not immediately obvious to me why the python signature for Add needs to accept the System<U>* as a pointer instead of a reference.

Done.
Given that this is the third or fourth location that this pattern appears, got rid of an old TODO to make WrapCallbacks. Explanation is in the comments there.

---

bindings/pydrake/systems/scalar_conversion.py, line 47 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Do we expect a pydrake user to call this method directly. If so, I think this doc is quite a bit too obscure and brief. Even as domain expert trying to read the code, I am pretty confused by this whole file.

Done. Unfortunately, I'm not sure how to clarify the structure of it; however, I have made the documentation more example-based.

Users will unfortunately need this for defining scalar-covertible Python classes, but we can always author a trampoline to minimize the pain.

---

systems/framework/system_scalar_converter.h, line 104 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This phrasing could be slightly improved to more clearly be an instruction to an author changing this list, rather than a reader trying to understand it (i.e., this could be misread as explaining what is happening). Maybe more like "When changing the pairs of supported types below, be sure to also change foo to be the same list of pairs".

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 8 of 10 files at r2.
Review status: 9 of 11 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/systems/scalar_conversion.py, line 26 at r3 (raw file):

class ScalarHelper(object):
    """A helper to handle dispatching constructors and help creating
    converters."""

I am not loving the way this ends up working on for users... the example below is fairly convoluted and unpythonic. Can we chat f2f sometime about the background here and some alternatives? My intuition points more towards (1) a way to declare the class at the top level instead of nested -- even if we still also have the def function wrapping it, and (2) always requiring the user to write regular ctor and copy ctor separately, with magic name(s), instead of calling "check_if_copying" explicitly.

---

bindings/pydrake/util/wrap_pybind.h, line 51 at r3 (raw file):

// TODO(eric.cousineau): Only use this on non-primitive types (e.g. types whose
// `type_caster`s are generic).

FYI Is this TODO already fixed by the is_generic_pybind check?

---

bindings/pydrake/util/wrap_pybind.h, line 77 at r3 (raw file):

/// Arguments with const or mutable references to objects that may only exist
/// in C++, and not yet in Python, may cause `pybind11` to trigger a copy, when
/// it's only valid to use a reference.

BTW While its clear to me in the context of this PR, in the future it might not be clear to a unprimed reader that this sentence is trying to explain why WrapCallbacks is useful or should be used, as opposed to explaining some API contract or semantic detail of the implementation.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/scalar_conversion.py, line 26 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not loving the way this ends up working on for users... the example below is fairly convoluted and unpythonic. Can we chat f2f sometime about the background here and some alternatives? My intuition points more towards (1) a way to declare the class at the top level instead of nested -- even if we still also have the def function wrapping it, and (2) always requiring the user to write regular ctor and copy ctor separately, with magic name(s), instead of calling "check_if_copying" explicitly.

OK Definitely up for f2f! Writing out some initial thoughts:

For (1), I don't think there will be a way to make a good and useful class-generator without nesting (constrained by decorators and Python's parsing), and I don't think the class nesting is that high of a price to pay.
If we want something sweeter, e.g. a trampoline / proxy setup for maximal type erasure, I think that should be handled downstream of this PR (or at a later point, when a fully Pythonic API is defined).

I've looked at things like metaclass: this could give us the ability to maybe generate classes, but at the price of (a) having a really shifty lexical scope, (b) not having the class identity well-defined as part of [a], and (c) potentially making typing much more complex than it needs to be.

For (2), yup, I think that can be done.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/scalar_conversion.py, line 26 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Definitely up for f2f! Writing out some initial thoughts:

For (1), I don't think there will be a way to make a good and useful class-generator without nesting (constrained by decorators and Python's parsing), and I don't think the class nesting is that high of a price to pay.
If we want something sweeter, e.g. a trampoline / proxy setup for maximal type erasure, I think that should be handled downstream of this PR (or at a later point, when a fully Pythonic API is defined).

I've looked at things like metaclass: this could give us the ability to maybe generate classes, but at the price of (a) having a really shifty lexical scope, (b) not having the class identity well-defined as part of [a], and (c) potentially making typing much more complex than it needs to be.

For (2), yup, I think that can be done.

Aha, I tried (1) locally and see the challenge now. We need to know what T is before we can refer to the correct superclass.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 7 of 12 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/systems/scalar_conversion.py, line 26 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Aha, I tried (1) locally and see the challenge now. We need to know what T is before we can refer to the correct superclass.

Done. Gave a shot at condensing the boiler-platey things:

• Made TemplateClass.define defer instantiation until they're requested. This permits referring to the template inside the function, and gets rid of that extra ugly argument.
• Made TemplateSystem.define, made more comprehensive tests (positive and negative). (Can rename to define_system_template, since it's kinda generic.
• Added test capturing inheritance case, using same propagation as C++.

---

bindings/pydrake/util/wrap_pybind.h, line 51 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Is this TODO already fixed by the is_generic_pybind check?

Done. D'oh!

---

bindings/pydrake/util/wrap_pybind.h, line 77 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW While its clear to me in the context of this PR, in the future it might not be clear to a unprimed reader that this sentence is trying to explain why WrapCallbacks is useful or should be used, as opposed to explaining some API contract or semantic detail of the implementation.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint. The shape of the API and example seem better.

FYI The code is subtle and complicated enough that its taking me a while to read through it all. I'll keep pushing on it.

---

Reviewed 2 of 6 files at r4, 1 of 3 files at r5.
Review status: 10 of 12 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/systems/scalar_conversion.py, line 34 at r5 (raw file):

    This differs from `TemplateClass` in that (a) you must pre-specify
    parameters at construction time and (b) `.define` is overridden to allow
    defining `f(T)` for convenience.

I don't understand what part (a) clause here is saying that I must do. Even in the example, I don't see what parameters are serving as "must specify". Perhaps it means that I can't refer to **kwargs in my _construct method?

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

def Example_(T):

    class ExampleInstantiation(LeafSystem_[T]):

Testing locally, instead of naming this ExampleInstantiation, I can name it Example or Impl and everything seems fine. What do you think about (here, and the API docs) suggesting one of those as a less-cumbersome pattern for users to employ?

I like Example because it's the class users are trying to write, but I also like Impl because its one less brittle duplication of the system name.

---

bindings/pydrake/util/cpp_template.py, line 109 at r5 (raw file):

        self._instantiation_map[param] = instantiation
        if instantiation is not TemplateBase._deferred:
            self._on_add(param, instantiation)

FYI I would have followed this better if add_instantiation_internal was defined after add_instantiation. I had to find the contract / invariants / preconditions in the public method before I could understand the private one.

---

bindings/pydrake/util/cpp_template.py, line 136 at r5 (raw file):

        """
        if self._instantiation_func is not None:
            # Ensure that we have all instantiations for the old function.

I am unable to foresee a case where someone calls add_instantiations more than once, and when whey do why it would be the case that forcing the prior deferred instantiations now is supposed to work, when it (presumably) would not have worked at the time the deferred entry was added. Maybe this case should generate an exception for now? Or else maybe add comments or better test cases that elucidate why this is relevant. (Test cases where the deferred instantiations are ints, instead of functions or classes, made it hard for me to use the test cases as an API guide here.)

---

bindings/pydrake/util/cpp_template.py, line 158 at r5 (raw file):

        for param in self.param_list:
            instantiation, _ = self.get_instantiation(param)
            if instantiation == obj:

BTW Unclear why we want == semantics here and not is semantics.

I guess when I reflect on it more I can see some reasons, but in any case for a method named is probably this deserves a comment explaining what's going on here.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 3 files at r5.
Review status: 11 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 39 at r5 (raw file):

            float, AutoDiffXd, Expression,
        )
        self.assertEqual(

FYI Meta I think unittest has a list-comparison primitive, with better error messages?

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 130 at r5 (raw file):

        with self.assertRaises(AssertionError):
            mut.TemplateSystem.define("C", T_list=T_list_bad)
        # - Not in original param list.

BTW By "param list" does this mean "T_list" specifically?

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 205 at r5 (raw file):

        with self.assertRaises(RuntimeError) as cm:
            NoConstructCopy_[float]
        self.assertIn(bad_init, str(cm.exception))

Hmm, why is the error message we expect here "should not define"?

---

Comments from Reviewable

[jwnimmer-tri]

First pass complete.

---

Reviewed 1 of 3 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/scalar_conversion.py, line 16 at r5 (raw file):

def _get_conversion_pairs(T_list):
    # Intersect.

If I am reading correctly, the T_compat filter is pointless. Needing it would imply that SupportedConversionPairs has scalars that are not in SupportedScalars. It seems like the T, U loop is all we need, checking that both T and U are in T_list.

---

bindings/pydrake/systems/scalar_conversion.py, line 39 at r5 (raw file):

    - Not define `__init__`, as this will be overridden.
    - Define `_construct(self, *args, converter=None)` and
      `_construct_copy(self, *args, converter=None)` instead.

Why should _construct_copy ever use *args? It seems like we'd always want other exactly.

---

bindings/pydrake/systems/scalar_conversion.py, line 56 at r5 (raw file):

                    self.value = value

                def _construct_copy(self, other, converter=None):

I am having a hard time understanding why converter should ever be non-None here? I see that sometimes it is (per the system_init helper function below), but that seems wrong -- the converter should either be implied by the type signature, or else aliased from other. It seems to me like the system_init could do the right thing here -- call the LeafSystem init for us already, with the correct converter, and then the only thing that our _construct has to do is establish our member fields and any other Declare calls. What I am I missing?

---

bindings/pydrake/systems/scalar_conversion.py, line 58 at r5 (raw file):

                def _construct_copy(self, other, converter=None):
                    LeafSystem_[T].__init__(self, converter)
                    self.value = other.value

To reduce brittleness, should this prefer to delegate instead?

 def _construct_copy(self, other, converter=None):
    self._construct(other.value, converter=converter)

That's what we recommend for C++ transmog ctors, and I think the rationale applies equally well here?

---

bindings/pydrake/systems/scalar_conversion.py, line 101 at r5 (raw file):

        # Add converter for later access.
        self._T_list = T_list
        self._T_pairs = T_pairs

FYI Should we copy the lists here? What's the python convention on list aliasing for passed arguments? Do we just presume that callers should never pass a list and then mutate it after we return?

---

bindings/pydrake/systems/scalar_conversion.py, line 137 at r5 (raw file):

        T, = param

        # Check that the user has not defined `__init__`, nad has defined

BTW typo

---

bindings/pydrake/systems/scalar_conversion.py, line 151 at r5 (raw file):

            raise RuntimeError(
                "Convertible systems should not define `__init__`, but must "
                "define `_construct` and `_construct_copy` instead.")

BTW Since this is likely to be a easy user mistake to make, I think we should have a specific, distinct error message for all three types of user error -- present init, missing constructor, missing construct_copy.

---

bindings/pydrake/systems/scalar_conversion.py, line 174 at r5 (raw file):

    def _check_if_copying(self, obj, *args, **kwargs):
        # Checks if a function signature implies a copy constructor.
        if len(args) >= 1:

Unclear when when len(args) > 1 that we should identify it as a copy ctor? In other words, I expected this to be == 1.

---

bindings/pydrake/systems/scalar_conversion.py, line 177 at r5 (raw file):

            if self.is_subclass_of_instantiation(type(args[0])):
                return True
        return False

FYI Is there anything we can check on kwargs to identify a copy? It seems like it should have converter only.

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Testing locally, instead of naming this ExampleInstantiation, I can name it Example or Impl and everything seems fine. What do you think about (here, and the API docs) suggesting one of those as a less-cumbersome pattern for users to employ?

I like Example because it's the class users are trying to write, but I also like Impl because its one less brittle duplication of the system name.

How about something like ExampleT, or ExampleImpl? I'm also up for Example, but that may get confusing if you have a default instantiation, and it's nice to have the closure still have a locally logical name.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

How about something like ExampleT, or ExampleImpl? I'm also up for Example, but that may get confusing if you have a default instantiation, and it's nice to have the closure still have a locally logical name.

Hm, what about just Instantiation then? I will mark myself resolved, but in general I think that copy-pasta of the class name is going to get annoying fast.

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hm, what about just Instantiation then? I will mark myself resolved, but in general I think that copy-pasta of the class name is going to get annoying fast.

Hm... If completely generic names is acceptable, then then my preference would be towards your earlier suggestion, Impl - would that work?

---

Comments from Reviewable

[jwnimmer-tri]

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Hm... If completely generic names is acceptable, then then my preference would be towards your earlier suggestion, Impl - would that work?

Sure Impl (or other generic names) are good by me. It would also be fine to leave it and users give feedback, since mine is just one opinion or likely many.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 7 of 12 files reviewed at latest revision, 16 unresolved discussions.

---

bindings/pydrake/systems/scalar_conversion.py, line 16 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If I am reading correctly, the T_compat filter is pointless. Needing it would imply that SupportedConversionPairs has scalars that are not in SupportedScalars. It seems like the T, U loop is all we need, checking that both T and U are in T_list.

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 34 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't understand what part (a) clause here is saying that I must do. Even in the example, I don't see what parameters are serving as "must specify". Perhaps it means that I can't refer to **kwargs in my _construct method?

Done. Sorry, wording was ambiguous, tried to clarify. Can remove this clause if it's still unclear.

---

bindings/pydrake/systems/scalar_conversion.py, line 39 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why should _construct_copy ever use *args? It seems like we'd always want other exactly.

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 56 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am having a hard time understanding why converter should ever be non-None here? I see that sometimes it is (per the system_init helper function below), but that seems wrong -- the converter should either be implied by the type signature, or else aliased from other. It seems to me like the system_init could do the right thing here -- call the LeafSystem init for us already, with the correct converter, and then the only thing that our _construct has to do is establish our member fields and any other Declare calls. What I am I missing?

Done.
Sorry, that was a placeholder for what would ideally be a keyword-only argument. Pending Python3:
https://www.python.org/dev/peps/pep-3102/
Added comment.

---

bindings/pydrake/systems/scalar_conversion.py, line 58 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To reduce brittleness, should this prefer to delegate instead?

 def _construct_copy(self, other, converter=None):
    self._construct(other.value, converter=converter)

That's what we recommend for C++ transmog ctors, and I think the rationale applies equally well here?

Done. Good point!

---

bindings/pydrake/systems/scalar_conversion.py, line 101 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Should we copy the lists here? What's the python convention on list aliasing for passed arguments? Do we just presume that callers should never pass a list and then mutate it after we return?

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 137 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW typo

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 151 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Since this is likely to be a easy user mistake to make, I think we should have a specific, distinct error message for all three types of user error -- present init, missing constructor, missing construct_copy.

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 174 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear when when len(args) > 1 that we should identify it as a copy ctor? In other words, I expected this to be == 1.

Done.

---

bindings/pydrake/systems/scalar_conversion.py, line 177 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Is there anything we can check on kwargs to identify a copy? It seems like it should have converter only.

Done.

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 15 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Sure Impl (or other generic names) are good by me. It would also be fine to leave it and users give feedback, since mine is just one opinion or likely many.

Done.

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 39 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Meta I think unittest has a list-comparison primitive, with better error messages?

Done. Thanks!

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 130 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW By "param list" does this mean "T_list" specifically?

Done.

---

bindings/pydrake/systems/test/scalar_conversion_test.py, line 205 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hmm, why is the error message we expect here "should not define"?

Done.

---

bindings/pydrake/util/cpp_template.py, line 109 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI I would have followed this better if add_instantiation_internal was defined after add_instantiation. I had to find the contract / invariants / preconditions in the public method before I could understand the private one.

Done.

---

bindings/pydrake/util/cpp_template.py, line 136 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am unable to foresee a case where someone calls add_instantiations more than once, and when whey do why it would be the case that forcing the prior deferred instantiations now is supposed to work, when it (presumably) would not have worked at the time the deferred entry was added. Maybe this case should generate an exception for now? Or else maybe add comments or better test cases that elucidate why this is relevant. (Test cases where the deferred instantiations are ints, instead of functions or classes, made it hard for me to use the test cases as an API guide here.)

Done. (Generating exception.)

---

bindings/pydrake/util/cpp_template.py, line 158 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Unclear why we want == semantics here and not is semantics.

I guess when I reflect on it more I can see some reasons, but in any case for a method named is probably this deserves a comment explaining what's going on here.

Done. Was not being strict on equality mechanism, thanks!

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/systems/scalar_conversion.py, line 34 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Sorry, wording was ambiguous, tried to clarify. Can remove this clause if it's still unclear.

OK This is good now, thanks.

---

bindings/pydrake/systems/scalar_conversion.py, line 56 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done.
Sorry, that was a placeholder for what would ideally be a keyword-only argument. Pending Python3:
https://www.python.org/dev/peps/pep-3102/
Added comment.

OK I think my error in reasoning was that while LeafSystem's only ctor argument is converter (and thus system_init could directly call LeafSystem ctor, passing through the correct converter, without passing it via _construct_copy), this pattern is intended to also be used with System subclasses that inherit from things other than LeafSystem, which might have different constructor arguments besides the converter (e.g., an Adder has a vector width, etc.), so the call to LeafSystem init must be manifest in MySystem, and thus we must pass the converter around via user code.

It would be great to figure out some sugar where that's not required, but I can see why its difficult.

---

Comments from Reviewable

[EricCousineau-TRI]

-@sherm1
+@soonho-tri for platform review, please.

---

Review status: 7 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/systems/scalar_conversion.py, line 56 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I think my error in reasoning was that while LeafSystem's only ctor argument is converter (and thus system_init could directly call LeafSystem ctor, passing through the correct converter, without passing it via _construct_copy), this pattern is intended to also be used with System subclasses that inherit from things other than LeafSystem, which might have different constructor arguments besides the converter (e.g., an Adder has a vector width, etc.), so the call to LeafSystem init must be manifest in MySystem, and thus we must pass the converter around via user code.

It would be great to figure out some sugar where that's not required, but I can see why its difficult.

OK Good point! I've made some TODOs for now, as I'm not aware of how to do it without deprecated features. (We could pass the template itself around, but that still requires the user to propagate information.)

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 10 of 10 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[soonho-tri]

Reviewed 3 of 10 files at r2, 1 of 5 files at r6, 7 of 10 files at r7, 8 of 10 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

bindings/pydrake/systems/framework_py_systems.cc, line 56 at r8 (raw file):

    // Explicit forward constructors, as we want the protected
    // `SystemScalarConverter` constructor as well.

BTW, I think this comment should be placed after theLeafSystem() {} line?

---

bindings/pydrake/systems/framework_py_systems.cc, line 57 at r8 (raw file):

    // Explicit forward constructors, as we want the protected
    // `SystemScalarConverter` constructor as well.
    LeafSystemPublic() {}

BTW, trivial, LeafSystemPublic() = default;

---

bindings/pydrake/util/cpp_template.py, line 79 at r8 (raw file):

        return self.get_instantiation(param)[0]

    # Unique token to signify that this instantiation is deferred.

Can you add more high-level documentation such as 1) what is deferred instantiation/evaluation, 2) why do we need to handle? I was able to find some clues from #8666 (comment) but definitely would appreciate if you can provide more explanations.

---

bindings/pydrake/util/wrap_pybind.h, line 76 at r8 (raw file):

/// or mutable) to a C++ argument or return value.
/// Otherwise, `pybind11` may try and copy the object, will be bad if either
/// the type is a non-copyable or if you are trying to the mutate the object

BTW, to the mutate -> to mutate?

---

Comments from Reviewable

[soonho-tri]

---

Reviewed 2 of 10 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 9 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/systems/framework_py_systems.cc, line 56 at r8 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, I think this comment should be placed after theLeafSystem() {} line?

OK I'd prefer to keep it at the top, since we're not able to use using Base::Base. I've added a comment to clarify.

---

bindings/pydrake/systems/framework_py_systems.cc, line 57 at r8 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, trivial, LeafSystemPublic() = default;

Done.

---

bindings/pydrake/util/cpp_template.py, line 79 at r8 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Can you add more high-level documentation such as 1) what is deferred instantiation/evaluation, 2) why do we need to handle? I was able to find some clues from #8666 (comment) but definitely would appreciate if you can provide more explanations.

Done. Let me know if that helps!

---

bindings/pydrake/util/wrap_pybind.h, line 76 at r8 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, to the mutate -> to mutate?

Done.

---

Comments from Reviewable

[soonho-tri]

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/util/cpp_template.py, line 79 at r8 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Let me know if that helps!

Nice! Thank you!

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable