Suggestion for better error handling

[zplizzi]

I receive the error:

Traceback (most recent call last):
  File "test.py", line 32, in <module>
    model.bake()
  File "pomegranate/BayesianNetwork.pyx", line 343, in pomegranate.BayesianNetwork.BayesianNetwork.bake
KeyError: {
    "class" :"Distribution",
    "dtype" :"str",
    "name" :"DiscreteDistribution",
    "parameters" :[
        {
            "True" :0.7142857142857143,
            "False" :0.2857142857142857
        }
    ],
    "frozen" :false
}

When running the following code sample:

import pomegranate as pg

a = pg.DiscreteDistribution({"True": 5 / 7, "False": 2 / 7})
b = pg.DiscreteDistribution({"True": .5, "False": .5})

model = pg.BayesianNetwork("test")
s1 = pg.State(a, name="a")
s2 = pg.State(b, name="b")

c = pg.ConditionalProbabilityTable([
    ["True", "True", "True", 1],
    ["True", "True", "False", 0],
    ["True", "False", "True", 0],
    ["True", "False", "False", 1],
    ["False", "True", "True", 0],
    ["False", "True", "False", 1],
    ["False", "False", "True", 0],
    ["False", "False", "False", 1],
], [a, b])

s3 = pg.State(c, name="c")

model.add_states(s1, s2, s3)

# This is wrong
model.add_edge(a, c)
model.add_edge(b, c)
# Should be:
# model.add_edge(s1, s3)
# model.add_edge(s2, s3)

model.bake()

I have noted in the code sample the lines that caused the error and the correct version. However, I think there could be better checks and error messages to make it easier to debug this issue in the future. The current error message isn't very useful. Just checking that the argument to add_edge is of type State may be sufficient.

My pomegranate version is 0.10.0 on Python 3.6.4.

[jkleckner]

@zplizzi I actually have a branch with a 3 line patch for that test in hmm.pyx but never submitted a PR because I wasn't sure if duck-typing was preferred. But I would agree that detecting the error earlier and more clearly would be helpful.

Date:   Mon Apr 30 17:51:48 2018 -0700

   Error message if not State
   
   But pythonic may frown on this...

diff --git a/pomegranate/hmm.pyx b/pomegranate/hmm.pyx
index 13e410e..f9a436a 100644
--- a/pomegranate/hmm.pyx
+++ b/pomegranate/hmm.pyx
@@ -386,6 +386,9 @@ cdef class HiddenMarkovModel(GraphModel):
        None
        """

+        if not isinstance(state, State):
+            raise ValueError("hmm.add_state given an object that isn't a subclass of State:'{}'".format(str(state)))
+
        if state.name in self.state_names:
            raise ValueError("A state with name '{}' already exists".format(state.name))

[zplizzi]

Hmm, I'm no expert on the most pythonic way to do that, but it seems like it would be helpful to me!

[jmschrei]

Thank you for opening an issue. pomegranate has recently been rewritten from the ground up to use PyTorch instead of Cython (v1.0.0), and so all issues are being closed as they are likely out of date. Please re-open or start a new issue if a related issue is still present in the new codebase.