Optional Arguments #86
Conversation
|
Did some refactoring to reduce duplication and use more of pyparsing superpowers. @gchenfc check it out when you get a chance. 🙂 |
gtwrap/interface_parser/function.py
Outdated
| rule = ((Type.rule ^ TemplatedType.rule)("ctype") + | ||
| IDENT("name")).setParseAction(lambda t: Argument(t.ctype, t.name)) | ||
| rule = ((Type.rule ^ TemplatedType.rule)("ctype") + IDENT("name") + | ||
| Optional(EQUAL + (DEFAULT_ARG ^ Type.rule ^ TemplatedType.rule) + |
There was a problem hiding this comment.
Maybe in unit test point out which test corresponds to which rule (DEFAULT_ARG, type, templated).
gtwrap/interface_parser/function.py
Outdated
| self.default = tuple(default.asList()) | ||
| else: | ||
| # An empty string | ||
| self.default = default |
There was a problem hiding this comment.
Why not "" if it's really empty?
gtwrap/interface_parser/tokens.py
Outdated
| # Encapsulating type for numbers, and single and double quoted strings. | ||
| # The pyparsing_common utilities ensure correct coversion to the corresponding type. | ||
| # E.g. pyparsing_common.number will convert 3.1415 to a float type. | ||
| _type = (pyparsing_common.number ^ QuotedString('"') ^ QuotedString("'")) |
There was a problem hiding this comment.
_type is not a great name. Maybe NUMBER_OR_QUOTED_STRING?
There was a problem hiding this comment.
Renaming it to NUMBER_OR_STRING.
gtwrap/pybind_wrapper.py
Outdated
| py_args = [] | ||
| for arg in args_list.args_list: | ||
| if arg.default and isinstance(arg.default, str): | ||
| arg.default = f"\"{arg.default}\"" |
There was a problem hiding this comment.
I think f-strings are post 3.8.
| if method.name == 'print': | ||
| # Redirect stdout - see pybind docs for why this is a good idea: | ||
| # https://pybind11.readthedocs.io/en/stable/advanced/pycpp/utilities.html#capturing-standard-output-from-ostream | ||
| ret = ret.replace('self->', 'py::scoped_ostream_redirect output; self->') | ||
| ret = ret.replace('self->print', 'py::scoped_ostream_redirect output; self->print') |
There was a problem hiding this comment.
I don't quite understand your comment.
There was a problem hiding this comment.
Nvm saw the next comment
| [](const {cpp_class} &a) {{ | ||
| # Make __repr__() call print() internally | ||
| ret += '''{prefix}.def("__repr__", | ||
| [](const {cpp_class}& self{opt_comma}{args_signature_with_names}){{ | ||
| gtsam::RedirectCout redirect; |
There was a problem hiding this comment.
then why do we still want this?
There was a problem hiding this comment.
print_ prints to (python) stdout whereas __repr__ returns a string. We could have print_ be like py::print(__repr__(self)) or something but I don't see a huge advantage to doing that.
There was a problem hiding this comment.
Sorry, I mean if we use py::scoped_ostream_redirect then we don't need to use the GTSAM redirect anymore.
There was a problem hiding this comment.
We should tackle this in another PR so that we don't inadvertently break something. 😕
| function varargout = print(this, varargin) | ||
| % PRINT usage: print(string s, KeyFormatter keyFormatter) : returns void | ||
| % Doxygen can be found at https://gtsam.org/doxygen/ | ||
| if length(varargin) == 2 && isa(varargin{1},'char') && isa(varargin{2},'gtsam.KeyFormatter') |
There was a problem hiding this comment.
It's interesting that the print method is here. I think by default it should use obj.display, but maybe that does not support custom KeyFormatter.
There was a problem hiding this comment.
I am not even touching the Matlab code right now because I am not the right person to do that.
gtwrap/interface_parser/function.py
Outdated
| IDENT("name")).setParseAction(lambda t: Argument(t.ctype, t.name)) | ||
| rule = ((Type.rule ^ TemplatedType.rule)("ctype") + IDENT("name") + | ||
| Optional(EQUAL + (DEFAULT_ARG ^ Type.rule ^ TemplatedType.rule) + | ||
| Optional(LPAREN + RPAREN))("default") |
There was a problem hiding this comment.
is this optional used anywhere?
There was a problem hiding this comment.
Yes it is. It is required to parse default constructors, e.g. = NonlinearFactorGraph() since Type will only parse NonlinearFactorGraph.
Added a comment.
gtwrap/interface_parser/function.py
Outdated
| rule = ((Type.rule ^ TemplatedType.rule)("ctype") + | ||
| IDENT("name")).setParseAction(lambda t: Argument(t.ctype, t.name)) | ||
| rule = ((Type.rule ^ TemplatedType.rule)("ctype") + IDENT("name") + | ||
| Optional(EQUAL + (DEFAULT_ARG ^ Type.rule ^ TemplatedType.rule) + |
There was a problem hiding this comment.
Should this maybe be
Optional( EQUAL +
(DEFAULT_ARG ^
( (Type.rule ^ TemplatedType.rule) + Optional(LPAREN + RPAREN) )
)
)???
i.e. It can either be a default arg, or a constructor?
Also maybe Optional(LPAREN + DEFAULT_ARG + RPAREN) ? so that the constructor can take constant arguments?
Actually there could even be nested constructors... oh jeez this is getting really involved
There was a problem hiding this comment.
The rule handles the left and right parens properly, check out the tests. I've added a check where if it is a Type, then to add back the parens.
My design sense says not to support non-default constructors. If the user wanted non-default constructors, then why use a default argument in the first place?
There was a problem hiding this comment.
That being said, maybe we can replace Type/TemplatedType with a Constructor rule?
There was a problem hiding this comment.
If the constructor rule supports templating*
This PR is a first-pass approach at optional arguments. It can recognize expressions such as:
in the interface file, and converts them with the appropriate pybind notation
This should make wrapping GTdynamics with the correct keyformatters a little nicer.
Although it is true that this could be done just as well with overloading and defining appropriate optional arguments in C++, implementing these optional arguments isn't too much code and should make code a little cleaner.