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

Wrapped classes in discrete #967

Merged
merged 23 commits into from
Dec 17, 2021
Merged

Wrapped classes in discrete #967

merged 23 commits into from
Dec 17, 2021

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
Collaborator

@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
Collaborator

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
Collaborator

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/Signature.h Outdated Show resolved Hide resolved
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
Collaborator

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
Collaborator

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.

gtsam/discrete/DiscreteConditional.h Outdated Show resolved Hide resolved
gtsam/discrete/discrete.i Outdated Show resolved Hide resolved
python/gtsam/specializations/discrete.h Show resolved Hide resolved
@dellaert dellaert changed the title Added wrapper files Wrapped classes in discrete Dec 16, 2021
@varunagrawal
Copy link
Collaborator

@dellaert I fixed the CI issue in #968

@varunagrawal
Copy link
Collaborator

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.

3 participants