-
Notifications
You must be signed in to change notification settings - Fork 767
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
Wrapped classes in discrete #967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM module some comments. I'll pull this branch and investigate what the CI failure is.
gtsam/discrete/DiscreteFactorGraph.h
Outdated
double operator()(const DiscreteValues& values) const; | ||
|
||
/// Synonym for operator(), mostly for wrapper | ||
double evaluate(const DiscreteValues& values) const { return operator()(values); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrapper supports operator overloading. I am pretty sure we added support for the callable operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax would be double operator()(const gtsam::DiscreteValues& values) const;
I used this in the DecisionTreeFactor
and it works as expected. 🙂
In [1]: f = gtsam.DecisionTreeFactor()
In [2]: f
Out[2]:
DecisionTreeFactor
Potentials:
Cardinalities: {}
Leaf 1
In [3]: f()
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-3-c43e34e6d405> in <module>
----> 1 f()
TypeError: __call__(): incompatible function arguments. The following argument types are supported:
1. (self: gtsam.gtsam.DiscreteFactor, arg0: gtsam::Assignment<unsigned long>) -> float
Invoked with: DecisionTreeFactor
Potentials:
Cardinalities: {}
Leaf 1
In [4]: values = gtsam.DiscreteValues()
In [5]: f(values)
Out[5]: 1.0
In [6]:
gtsam/discrete/discrete.i
Outdated
const gtsam::DecisionTreeFactor& marginal, | ||
const gtsam::Ordering& orderedKeys); | ||
size_t size() const; // TODO(dellaert): why do I have to repeat??? | ||
double evaluate(const gtsam::DiscreteValues& values) const; // TODO(dellaert): why do I have to repeat??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repetition is because while the underlying C++ code will call the proper super class method, the pybind11 wrapper needs to know that this is a method associated with this class. It is inefficient for the writer (you in this case) but it serves as a great index of what methods are applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOOOOO! That worked before. So, the semantics changed... The wrapper should generate all wrapped functions in base classes I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Okay then, we'll log this as a feature request.
I also added the GTSAM_EXPORT declarations in #968 so that Windows is happy. |
This adds python/MATLAB wrappers for the classes in discrete. @ProfFan helped with some of the wrapper magic.
Development was driven by
To make this possible, I had to do some constructor forwarding etc, but the main new functionality is the ability to construct DiscreteConditionals with a simpler constructor that could be wrapped. You can now do
which complements the magical sugar that was there before:
I also added a variant of DiscreteConditional::choose that returns a DecisionTreeFactor.
It works. Here is code that builds the Asia network:
print(asia)
yields: