Skip to content

Conversation

@dellaert
Copy link
Member

This adds python/MATLAB wrappers for the classes in discrete. @ProfFan helped with some of the wrapper magic.

Development was driven by

  • test_DiscreteFactorGraph.py
  • test_DiscreteBayesNet.py

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

  DiscreteConditional pXE(X, {E}, "95/5 2/98");

which complements the magical sugar that was there before:

  DiscreteConditional pXE(X | E = "95/5 2/98");

I also added a variant of DiscreteConditional::choose that returns a DecisionTreeFactor.

It works. Here is code that builds the Asia network:

asia = DiscreteBayesNet()
asia.add(Asia, P([]), "99/1")
asia.add(Smoking, P([]), "50/50")

asia.add(Tuberculosis, P([Asia]), "99/1 95/5")
asia.add(LungCancer, P([Smoking]), "99/1 90/10")
asia.add(Bronchitis, P([Smoking]), "70/30 40/60")

asia.add(Either, P([Tuberculosis, LungCancer]), "F T T T")

asia.add(XRay, P([Either]), "95/5 2/98")
asia.add(Dyspnea, P([Either, Bronchitis]), "9/1 2/8 3/7 1/9")

print(asia) yields:

DiscreteBayesNet

size: 8
factor 0:  P( 0 )
  Cardinalities: {0:2, }
  Choice(0) 
  0 Leaf 0.99
  1 Leaf 0.01

factor 1:  P( 4 )
  Cardinalities: {4:2, }
  Leaf 0.5

factor 2:  P( 3 | 0 )
  Cardinalities: {0:2, 3:2, }
  Choice(3) 
  0 Choice(0) 
  0 0 Leaf 0.99
  0 1 Leaf 0.95
  1 Choice(0) 
  1 0 Leaf 0.01
  1 1 Leaf 0.05

factor 3:  P( 6 | 4 )
  Cardinalities: {4:2, 6:2, }
  Choice(6) 
  0 Choice(4) 
  0 0 Leaf 0.99
  0 1 Leaf 0.9
  1 Choice(4) 
  1 0 Leaf 0.01
  1 1 Leaf 0.1

factor 4:  P( 7 | 4 )
  Cardinalities: {4:2, 7:2, }
  Choice(7) 
  0 Choice(4) 
  0 0 Leaf 0.7
  0 1 Leaf 0.4
  1 Choice(4) 
  1 0 Leaf 0.3
  1 1 Leaf 0.6

factor 5:  P( 5 | 3 6 )
  Cardinalities: {3:2, 5:2, 6:2, }
  Choice(6) 
  0 Choice(5) 
  0 0 Choice(3) 
  0 0 0 Leaf 1
  0 0 1 Leaf 0
  0 1 Choice(3) 
  0 1 0 Leaf 0
  0 1 1 Leaf 1
  1 Choice(5) 
  1 0 Leaf 0
  1 1 Leaf 1

factor 6:  P( 2 | 5 )
  Cardinalities: {2:2, 5:2, }
  Choice(5) 
  0 Choice(2) 
  0 0 Leaf 0.95
  0 1 Leaf 0.05
  1 Choice(2) 
  1 0 Leaf 0.02
  1 1 Leaf 0.98

factor 7:  P( 1 | 5 7 )
  Cardinalities: {1:2, 5:2, 7:2, }
  Choice(7) 
  0 Choice(5) 
  0 0 Choice(1) 
  0 0 0 Leaf 0.9
  0 0 1 Leaf 0.1
  0 1 Choice(1) 
  0 1 0 Leaf 0.3
  0 1 1 Leaf 0.7
  1 Choice(5) 
  1 0 Choice(1) 
  1 0 0 Leaf 0.2
  1 0 1 Leaf 0.8
  1 1 Choice(1) 
  1 1 0 Leaf 0.1
  1 1 1 Leaf 0.9

Copy link
Contributor

@varunagrawal varunagrawal left a 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.

double operator()(const DiscreteValues& values) const;

/// Synonym for operator(), mostly for wrapper
double evaluate(const DiscreteValues& values) const { return operator()(values); }
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example?

Copy link
Contributor

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]: 

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???
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

@dellaert dellaert changed the title Added wrapper files Wrapped classes in discrete Dec 16, 2021
@varunagrawal
Copy link
Contributor

@dellaert I fixed the CI issue in #968

@varunagrawal
Copy link
Contributor

I also added the GTSAM_EXPORT declarations in #968 so that Windows is happy.

@dellaert dellaert merged commit 0bab7b0 into develop Dec 17, 2021
@dellaert dellaert deleted the feature/discrete_wrapper branch December 17, 2021 02:20
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.

4 participants