cpp_template: Add Python mechanisms to pave way for C++ template binding.

[EricCousineau-TRI]

Related to exposing templates in #7660.

Final PR is to combine with #7732 and incorporate C++ glue:
https://github.com/EricCousineau-TRI/repro/blob/fa4e6a0/python/bindings/pymodule/tpl/cpp_template.h#L45

---

This change is 

[EricCousineau-TRI]

+@jadecastro for feature review, please (if it's not too much).
+@jwnimmer-tri for platform review, please.

---

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

---

Comments from Reviewable

[jwnimmer-tri]

Checkpoint.

---

Reviewed 2 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, 10 unresolved discussions.

---

bindings/pydrake/util/cpp_template.py, line 9 at r1 (raw file):

# Pending #7732.

BTW Should this be a TODO instead?

---

bindings/pydrake/util/cpp_template.py, line 14 at r1 (raw file):

def get_types_canonical(param):

I was confused because I didn't initially understand that this was a stub. Needs a comment like the 7732 above.

---

bindings/pydrake/util/cpp_template.py, line 25 at r1 (raw file):

def get_or_init(scope, name, template_cls, *args, **kwargs):
    """Gets an existing template from a scope if it exists; otherwise, it will
    be created and registered. """

FYI Meta I think the """ nominally belongs on the following line?

---

bindings/pydrake/util/cpp_template.py, line 25 at r1 (raw file):

def get_or_init(scope, name, template_cls, *args, **kwargs):
    """Gets an existing template from a scope if it exists; otherwise, it will
    be created and registered. """

This appears to be a public API (no leading _), but I don't think its clear what the three arguments denote (or where args and kwargs forwarding goes).

---

bindings/pydrake/util/cpp_template.py, line 26 at r1 (raw file):

    """Gets an existing template from a scope if it exists; otherwise, it will
    be created and registered. """
    tpl = getattr(scope, name, None)

BTW Disallowed abbreviation.

---

bindings/pydrake/util/cpp_template.py, line 37 at r1 (raw file):

class Template(object):

FYI Should this base class ever be used directly at call sites, or is it merely convenient implementation reuse? If the latter, consider a rename to TemplateBase to make that clear.

---

bindings/pydrake/util/cpp_template.py, line 56 at r1 (raw file):

        Can be one of the following forms:
            tpl[param0]
            tpl[param0, param1, ...]

I am not sure what this line means, given that param is a single item and we're not using *args. (At least, I couldn't find a test case that matches this signature.)

---

bindings/pydrake/util/cpp_template.py, line 89 at r1 (raw file):

        self.param_list.append(param)
        self._instantiation_map[param] = instantiation
        if self._param_default == _PARAM_DEFAULT:

BTW Ah, I see what this is doing now. The default isn't a so much a default (that sticks around, and has a meaning on its own), its a special value that means "not yet set" ala C++ optional. Consider if a different name would help clarify.

---

bindings/pydrake/util/cpp_template.py, line 95 at r1 (raw file):

    def add_instantiations(
            self, instantiation_func, param_list=None):

BTW I don't understand why param_list is given a default.

---

bindings/pydrake/util/cpp_template.py, line 117 at r1 (raw file):

        if param is None:
            assert self._param_default is not None
            param = self._param_default

Should this allow self._param_default to be the magic _PARAM_DEFAULT value? That seems like not what the resolver should be doing? Or if so, needs some more documentation.

---

Comments from Reviewable

[jadecastro]

Pass complete.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

bindings/pydrake/util/cpp_template.py, line 122 at r1 (raw file):

                # Assume scalar.
                param = (param,)
            else:

BTW, I don't think this branch is checked in the unit tests?

---

bindings/pydrake/util/cpp_template.py, line 138 at r1 (raw file):

    def _on_add(self, param, instantiation):
        # To be overrode by child classes.

BTW ("overrode by" -> "overridden by")

---

bindings/pydrake/util/cpp_template.py, line 159 at r1 (raw file):

class TemplateMethod(TemplateFunction):

BTW, can this just derive directly from the Template class, removing TemplateFunction, or is there some intended use case I'm missing?

---

Comments from Reviewable

[jadecastro]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

bindings/pydrake/util/test/cpp_template_test.py, line 10 at r1 (raw file):

class A(object):

BTW, please label these "dummy" classes and functions here and below.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/util/cpp_template.py, line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Should this be a TODO instead?

Done.

---

bindings/pydrake/util/cpp_template.py, line 14 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I was confused because I didn't initially understand that this was a stub. Needs a comment like the 7732 above.

Done.

---

bindings/pydrake/util/cpp_template.py, line 25 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Meta I think the """ nominally belongs on the following line?

Done.

---

bindings/pydrake/util/cpp_template.py, line 25 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This appears to be a public API (no leading _), but I don't think its clear what the three arguments denote (or where args and kwargs forwarding goes).

Done.

---

bindings/pydrake/util/cpp_template.py, line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Disallowed abbreviation.

Done.

---

bindings/pydrake/util/cpp_template.py, line 37 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Should this base class ever be used directly at call sites, or is it merely convenient implementation reuse? If the latter, consider a rename to TemplateBase to make that clear.

Done.

---

bindings/pydrake/util/cpp_template.py, line 56 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure what this line means, given that param is a single item and we're not using *args. (At least, I couldn't find a test case that matches this signature.)

Done. Removed param_default, simplified to allow_default, with comments indicating what this means.

---

bindings/pydrake/util/cpp_template.py, line 89 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Ah, I see what this is doing now. The default isn't a so much a default (that sticks around, and has a meaning on its own), its a special value that means "not yet set" ala C++ optional. Consider if a different name would help clarify.

Done.

---

bindings/pydrake/util/cpp_template.py, line 95 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I don't understand why param_list is given a default.

Done. Oops! From an old version.

---

bindings/pydrake/util/cpp_template.py, line 117 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Should this allow self._param_default to be the magic _PARAM_DEFAULT value? That seems like not what the resolver should be doing? Or if so, needs some more documentation.

Done. Removed _param_default.

---

bindings/pydrake/util/cpp_template.py, line 122 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, I don't think this branch is checked in the unit tests?

Done. Added.

---

bindings/pydrake/util/cpp_template.py, line 138 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW ("overrode by" -> "overridden by")

Done.

---

bindings/pydrake/util/cpp_template.py, line 159 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, can this just derive directly from the Template class, removing TemplateFunction, or is there some intended use case I'm missing?

Done.

---

bindings/pydrake/util/test/cpp_template_test.py, line 10 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, please label these "dummy" classes and functions here and below.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 2 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/util/cpp_template.py, line 56 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

Done. Removed param_default, simplified to allow_default, with comments indicating what this means.

OK As it turns out, what I was missing here on first read is that in Python __getitem__ is passed a tuple if you do template[a, b] from calling code -- that it never receives multiple *args. TIL.

---

bindings/pydrake/util/cpp_template.py, line 23 at r2 (raw file):

def get_or_init(scope, name, template_cls, *args, **kwargs):
    """

BTW Annoying PEP-257 suggests that the """ be immediately followed by the opening sentence, rather than a blank line.

---

Comments from Reviewable

[jwnimmer-tri]

---

Reviewed 1 of 2 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/util/cpp_template.py, line 1 at r2 (raw file):

#!/usr/bin/env python

This doesn't have a __main __ function; thus it should not have a shebang.

---

bindings/pydrake/util/test/cpp_template_test.py, line 1 at r2 (raw file):

#!/usr/bin/env python

Python binaries compiled by Bazel should not have shebang lines.

---

bindings/pydrake/util/test/cpp_template_test.py, line 40 at r2 (raw file):

class TestCppTemplate(unittest.TestCase):
    def test_base(self):
        tpl = m.TemplateBase("BaseTpl")

Meta Some more template (confusing vs tuple) abbrevs here.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/util/cpp_template.py, line 56 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK As it turns out, what I was missing here on first read is that in Python __getitem__ is passed a tuple if you do template[a, b] from calling code -- that it never receives multiple *args. TIL.

Done.

---

bindings/pydrake/util/cpp_template.py, line 1 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This doesn't have a __main __ function; thus it should not have a shebang.

Done.

---

bindings/pydrake/util/cpp_template.py, line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Annoying PEP-257 suggests that the """ be immediately followed by the opening sentence, rather than a blank line.

Done.

---

bindings/pydrake/util/test/cpp_template_test.py, line 1 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Python binaries compiled by Bazel should not have shebang lines.

Done.

---

bindings/pydrake/util/test/cpp_template_test.py, line 40 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta Some more template (confusing vs tuple) abbrevs here.

Done.

I'd like to maintain a short suffix, as I'd like to use this with the systems framework. If you can think of another short convention (such as a trailing _, or just T), I'd be open to changing to that from Tpl.

Default instantiation would be ${name}, and then template would be ${name}${suffix}.
Options:
Adder == AdderTpl[float]
Adder == Adder_[float]
Adder == AdderT[float]

---

Comments from Reviewable

[jwnimmer-tri]

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

---

bindings/pydrake/util/test/cpp_template_test.py, line 40 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

I'd like to maintain a short suffix, as I'd like to use this with the systems framework. If you can think of another short convention (such as a trailing _, or just T), I'd be open to changing to that from Tpl.

Default instantiation would be ${name}, and then template would be ${name}${suffix}.
Options:
Adder == AdderTpl[float]
Adder == Adder_[float]
Adder == AdderT[float]

I think for this unit test the "Tpl" string is just fine (I only intended my comment to cover the tpl variable names, but didn't write it clearly). I think we can debate the framework's string names once we're considering that specific commit (either in PR form, or a pre-review design check).

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/util/test/cpp_template_test.py, line 40 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think for this unit test the "Tpl" string is just fine (I only intended my comment to cover the tpl variable names, but didn't write it clearly). I think we can debate the framework's string names once we're considering that specific commit (either in PR form, or a pre-review design check).

OK Sounds good!

---

Comments from Reviewable

[jadecastro]

!

---

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[EricCousineau-TRI]

Note: Updated since #7732 was merged.

---

Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[jadecastro]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable