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

pydrake cpp_template: Add decorator for ease of defining instantiations #8666

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Apr 24, 2018

Towards defining user classes with scalar type conversion, etc, in parallel with #8665.

\cc @gizatt @RussTedrake


This change is Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

+@avalenzu for feature review, please.
+@sammy-tri for platform review, please.


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


Comments from Reviewable

@avalenzu
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@sammy-tri
Copy link
Contributor

First pass complete. I don't think there's anything actually wrong here, just a couple of questions.


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


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

        Example:
        @TemplateClass.define("MyTemplate", param_list=[(int,), (float,)])

BTW I don't know if this is an issue here or on add_instantiations, but it's not clear to me how param_list is used.


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

        Example:
        @TemplateClass.define("MyTemplate", param_list=[(int,), (float,)])
        def MyTemplate(param):

BTW Is it worth explaining that there's a convention that the name of the decorated function should be the name argument to the define decorator? Or would that be super obvious to someone familiar with this code? (or is that not actually a convention at all?)


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

        self.assertIsInstance(MyTemplate, m.TemplateClass)
        MyClass = MyTemplate[None]  # Default instantiation.

It seems confusing to me to use MyClass as the name of both the inner class being returned by the decorated function and as the name of a variable here which refers to an instance of... I guess I'd call it an instance of MyTemplate? though that's just the name of a generator function... which is named to be like a class because it's kinda like a class in C++ which takes template arguments? My mind is a little bent, I feel like a python newb.


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/py_cpp_template_user branch from 3c11cff to 3784ad2 Compare April 24, 2018 18:03
@EricCousineau-TRI
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


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

Previously, sammy-tri (Sam Creasey) wrote…

BTW I don't know if this is an issue here or on add_instantiations, but it's not clear to me how param_list is used.

Done.


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

Previously, sammy-tri (Sam Creasey) wrote…

BTW Is it worth explaining that there's a convention that the name of the decorated function should be the name argument to the define decorator? Or would that be super obvious to someone familiar with this code? (or is that not actually a convention at all?)

Done. Tried to clarify, sorry about that!


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

Previously, sammy-tri (Sam Creasey) wrote…

It seems confusing to me to use MyClass as the name of both the inner class being returned by the decorated function and as the name of a variable here which refers to an instance of... I guess I'd call it an instance of MyTemplate? though that's just the name of a generator function... which is named to be like a class because it's kinda like a class in C++ which takes template arguments? My mind is a little bent, I feel like a python newb.

Done.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

:lgtm:


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


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Tried to clarify, sorry about that!

way clearer now. Thanks!


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Sorry to tack on one more thing, but in taking the scalar type conversion a little further, realized that the instantiation_func needs to be aware of the calling template.

Syntactically, decorated functions can refer to them, since the variable will be injected into the function's globals() *once the decorator has finished.

However, for this usage, the function is used before the decorator returns, so the variable does not exist.
This is one solution; the other is to just declare the TemplateClass before, and then make a decorator which just add instantiations.

Will quickly prototype the other solution and post it as a comparison.


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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Here's the alternative:
https://github.com/EricCousineau-TRI/drake/compare/feature/py_cpp_template_user...EricCousineau-TRI:feature/py_cpp_template_user-alt?expand=1

I'm game for either, as they're just sugar. Can I ask if either you have a preference?


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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

(Another alternative is to defer evaluation of instantiations using a queue until requested, which shouldn't be too hard.)


Comments from Reviewable

@sammy-tri
Copy link
Contributor

I think I'm fine with this implementation if you are.


Reviewed 2 of 2 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


bindings/pydrake/util/cpp_template.py, line 182 at r3 (raw file):

        Example:
        @TemplateClass.define("MyTemplate", param_list=[(int,), (float,)])
        def MyTemplate(param):

BTW I think this needs to be updated to match the change to add_instantiations?


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/py_cpp_template_user branch from 1ac95e4 to 33406cc Compare April 24, 2018 20:49
@EricCousineau-TRI
Copy link
Contributor Author

OK Sounds good!


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


bindings/pydrake/util/cpp_template.py, line 182 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

BTW I think this needs to be updated to match the change to add_instantiations?

Done.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

3 participants