Repository: pgmpy/pgmpy

Issue:
DynamicBayesianNetwork model function get_cdps does not gets all TabularCDPs
### Subject of the issue

Apparently, the model `pgmpy.models.DynamicBayesianNetwork` function `get_cpds(*)` ignores all `TabularCDP` with time index 1, for example:

`cdp_T  = TabularCPD(('s', 1), 1000,  T.T, [('s', 0)], [1000])`

### Your environment
- pgmpy version: 0.1.15
- Python version: 3.7.10
- Operating: System Microsoft Windows 10 Pro

### Steps to Reproduce

Example from the code:

```
from pgmpy.models import DynamicBayesianNetwork as DBN
from pgmpy.factors.discrete import TabularCPD

dbn = DBN()
dbn.add_edges_from([(('D', 0),('G', 0)),(('I', 0),('G', 0)),(('D', 0),('D', 1)),(('I', 0),('I', 1))])
grade_cpd = TabularCPD(('G', 0), 3, [[0.3, 0.05, 0.9,  0.5],
                                     [0.4, 0.25, 0.08, 0.3],
                                     [0.3, 0.7,  0.02, 0.2]],
                       evidence=[('I', 0),('D', 0)],
                       evidence_card=[2, 2])
d_i_cpd = TabularCPD(('D',1), 2, [[0.6, 0.3],
                                  [0.4, 0.7]],
                     evidence=[('D',0)],
                     evidence_card=[2])
diff_cpd = TabularCPD(('D', 0), 2, [[0.6], [0.4]])
intel_cpd = TabularCPD(('I', 0), 2, [[0.7], [0.3]])
i_i_cpd = TabularCPD(('I', 1), 2, [[0.5, 0.4],
                                   [0.5, 0.6]],
                     evidence=[('I', 0)],
                     evidence_card=[2])
dbn.add_cpds(grade_cpd, d_i_cpd, diff_cpd, intel_cpd, i_i_cpd)
dbn.get_cpds()
```

### Expected behavior
```
[<TabularCPD representing P(('G', 0):3 | ('I', 0):2, ('D', 0):2) at 0x7ff7f27b0cf8>,
 <TabularCPD representing P(('D', 1):2 | ('D', 0):2) at 0x7ff810b9c2e8>, 
 <TabularCPD representing P(('D', 0):2) at 0x7ff7f27e6f98>,
 <TabularCPD representing P(('I', 0):2) at 0x7ff7f27e6ba8>,
 <TabularCPD representing P(('I', 1):2 | ('I', 0):2) at 0x7ff7f27e6668>]
```

### Actual behavior

```
[<TabularCPD representing P(('D', 0):2) at 0x21ee2840d48>,
 <TabularCPD representing P(('G', 0):3 | ('I', 0):2, ('D', 0):2) at 0x21ee2840848>,
 <TabularCPD representing P(('I', 0):2) at 0x21ee28400c8>]
```

Note all `TabularCPD representing P(('Something', 1):X ...)` is not included to the model. There is another example:

### Other example

```
from pgmpy.models import DynamicBayesianNetwork as DBN
from pgmpy.factors.discrete import TabularCPD

dbn = DBN([(('s', 0), ('s', 1)), (('s', 1), ('o', 1))])
# dbn.add_edges_from([(('s', 0), ('s', 1))])
cdp_s0 = TabularCPD(('s', 0), 2, [[0.2],[0.8]])
cdp_T  = TabularCPD(('s', 1), 2, [[0.5,0.3],[0.5,0.7]], [('s', 0)], [2])
cdp_o  = TabularCPD(('o', 1), 2, [[0.1,0.9],[0.9,0.1]], [('s', 1)], [2])
dbn.add_cpds(cdp_s0, cdp_T, cdp_o)
dbn.get_cpds()
```
It should:

```
[<TabularCPD representing P(('s', 0):2) at 0x246c03ccf08>,
 <TabularCPD representing P(('s', 1):2 | ('s', 0):2) at 0x246c02b7508>,
 <TabularCPD representing P(('o', 1):2 | ('s', 1):2) at 0x246c02b7808>]
```

But it gives:
```
[<TabularCPD representing P(('s', 0):2) at 0x246c02b8bc8>]
```






Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy) Report
> Merging [#1450](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy) (d54157b) into [dev](https://codecov.io/gh/pgmpy/pgmpy/commit/7b691d27f0dc39999db1a44a186813e42c515f7c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy) (7b691d2) will **decrease** coverage by `0.79%`.
> The diff coverage is `72.22%`.

[![Impacted file tree graph](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/graphs/tree.svg?width=650&height=150&src=pr&token=UaJMCdHaEF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy)](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy)

```diff
@@            Coverage Diff             @@
##              dev    #1450      +/-   ##
==========================================
- Coverage   94.56%   93.76%   -0.80%     
==========================================
  Files         141      141              
  Lines       15828    15840      +12     
==========================================
- Hits        14967    14852     -115     
- Misses        861      988     +127     
```


| [Impacted Files](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy) | Coverage Δ | |
|---|---|---|
| [pgmpy/models/DynamicBayesianNetwork.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvbW9kZWxzL0R5bmFtaWNCYXllc2lhbk5ldHdvcmsucHk=) | `91.34% <70.58%> (-1.81%)` | :arrow_down: |
| [...y/tests/test\_models/test\_DynamicBayesianNetwork.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvdGVzdHMvdGVzdF9tb2RlbHMvdGVzdF9EeW5hbWljQmF5ZXNpYW5OZXR3b3JrLnB5) | `100.00% <100.00%> (ø)` | |
| [pgmpy/readwrite/XMLBeliefNetwork.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvcmVhZHdyaXRlL1hNTEJlbGllZk5ldHdvcmsucHk=) | `53.62% <0.00%> (-39.14%)` | :arrow_down: |
| [...gmpy/tests/test\_readwrite/test\_XMLBeliefNetwork.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvdGVzdHMvdGVzdF9yZWFkd3JpdGUvdGVzdF9YTUxCZWxpZWZOZXR3b3JrLnB5) | `75.40% <0.00%> (-18.86%)` | :arrow_down: |
| [pgmpy/readwrite/PomdpX.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvcmVhZHdyaXRlL1BvbWRwWC5weQ==) | `86.62% <0.00%> (-10.37%)` | :arrow_down: |
| [pgmpy/tests/test\_readwrite/test\_PomdpX.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvdGVzdHMvdGVzdF9yZWFkd3JpdGUvdGVzdF9Qb21kcFgucHk=) | `83.68% <0.00%> (-9.22%)` | :arrow_down: |
| [pgmpy/estimators/MmhcEstimator.py](https://codecov.io/gh/pgmpy/pgmpy/pull/1450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy#diff-cGdtcHkvZXN0aW1hdG9ycy9NbWhjRXN0aW1hdG9yLnB5) | `98.11% <0.00%> (-1.89%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy). Last update [7b691d2...d54157b](https://codecov.io/gh/pgmpy/pgmpy/pull/1450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pgmpy).

ankurankan: @FelipeGiro If `test_CITest` is failing locally for you, it's probably because of an older version of scipy. Upgrading scipy should fix it.

Patch:
diff --git a/pgmpy/models/DynamicBayesianNetwork.py b/pgmpy/models/DynamicBayesianNetwork.py
index 2c66813b..b9451614 100644
--- a/pgmpy/models/DynamicBayesianNetwork.py
+++ b/pgmpy/models/DynamicBayesianNetwork.py
@@ -32,7 +32,7 @@ class DynamicNode:
         return f"({self.node}, {self.time_slice})"
 
     def __repr__(self) -> str:
-        return f"<DiscreteNode({self.node}, {self.time_slice}) at {hex(id(self))}>"
+        return f"<DynamicNode({self.node}, {self.time_slice}) at {hex(id(self))}>"
 
     def __lt__(self, other) -> bool:
         if self.node < other.node:
@@ -195,6 +195,16 @@ class DynamicBayesianNetwork(DAG):
             )
         )
 
+    def _timeslices(self):
+        return list(
+            set(
+                [
+                    timeslice
+                    for node, timeslice in super(DynamicBayesianNetwork, self).nodes()
+                ]
+            )
+        )
+
     def add_edge(self, start, end, **kwargs):
         """
         Add an edge between two nodes.
@@ -462,7 +472,7 @@ class DynamicBayesianNetwork(DAG):
 
         self.cpds.extend(cpds)
 
-    def get_cpds(self, node=None, time_slice=0):
+    def get_cpds(self, node=None, time_slice=None):
         """
         Returns the CPDs that have been associated with the network.
 
@@ -489,7 +499,23 @@ class DynamicBayesianNetwork(DAG):
         >>> dbn.add_cpds(grade_cpd)
         >>> dbn.get_cpds()
         """
-        # TODO: fix bugs in this
+
+        if time_slice is None:
+            time_slices = self._timeslices()
+        elif isinstance(time_slice, int) and time_slice >= 0:
+            time_slices = [time_slice]
+        elif isinstance(time_slice, typing.Iterable):
+            if all(isinstance(n, int) for n in time_slice):
+                time_slices = time_slice
+            else:
+                raise ValueError(
+                    "At least one element inside time_slice interable is not positive and/or integer"
+                )
+        else:
+            raise ValueError(
+                "Time slice is not a positive integer neither a interable of integers"
+            )
+
         if node:
             if node not in super(DynamicBayesianNetwork, self).nodes():
                 raise ValueError("Node not present in the model.")
@@ -499,10 +525,11 @@ class DynamicBayesianNetwork(DAG):
                         return cpd
         else:
             return_cpds = []
-            for var in self.get_slice_nodes(time_slice=time_slice):
-                cpd = self.get_cpds(node=var)
-                if cpd:
-                    return_cpds.append(cpd)
+            for time_slice in time_slices:
+                for var in self.get_slice_nodes(time_slice=time_slice):
+                    cpd = self.get_cpds(node=var)
+                    if cpd:
+                        return_cpds.append(cpd)
             return return_cpds
 
     def remove_cpds(self, *cpds):
@@ -757,9 +784,7 @@ class DynamicBayesianNetwork(DAG):
         """
         from pgmpy.models import BayesianNetwork
 
-        return BayesianNetwork(
-            ebunch=[(f"{u[0]}_{u[1]}", f"{v[0]}_{v[1]}") for (u, v) in self.edges()]
-        )
+        return BayesianNetwork(ebunch=self.edges())
 
     def fit(self, data, estimator="MLE"):
         """
@@ -824,7 +849,9 @@ class DynamicBayesianNetwork(DAG):
             colnames.extend([(node, t_slice + 1) for node in self._nodes()])
 
             df_slice = data.loc[:, colnames]
-            new_colnames = [f"{node}_{t - t_slice}" for (node, t) in df_slice.columns]
+            new_colnames = [
+                DynamicNode(node, t - t_slice) for (node, t) in df_slice.columns
+            ]
             df_slice.columns = new_colnames
             if t_slice == 0:
                 const_bn.fit(df_slice)
@@ -833,14 +860,12 @@ class DynamicBayesianNetwork(DAG):
 
         cpds = []
         for cpd in const_bn.cpds:
-            variables = [var.split("_") for var in cpd.variables]
-            variables = [(v, int(t)) for v, t in variables]
             cpds.append(
                 TabularCPD(
-                    variable=variables[0],
+                    variable=cpd.variables[0],
                     variable_card=cpd.variable_card,
                     values=cpd.get_values(),
-                    evidence=variables[1:],
+                    evidence=cpd.variables[1:],
                     evidence_card=cpd.cardinality[1:],
                 )
             )


Test patch:
diff --git a/pgmpy/tests/test_models/test_DynamicBayesianNetwork.py b/pgmpy/tests/test_models/test_DynamicBayesianNetwork.py
index c189a875..09082f4c 100644
--- a/pgmpy/tests/test_models/test_DynamicBayesianNetwork.py
+++ b/pgmpy/tests/test_models/test_DynamicBayesianNetwork.py
@@ -211,13 +211,24 @@ class TestDynamicBayesianNetworkMethods(unittest.TestCase):
         )
         self.network.initialize_initial_state()
         self.assertEqual(
-            set(self.network.get_cpds()),
+            {cpd.variable for cpd in self.network.get_cpds()},
+            {("I", 1), ("D", 0), ("G", 1), ("I", 0), ("G", 0), ("D", 1)},
+        )
+        self.assertEqual(
+            {cpd.variable for cpd in self.network.get_cpds(time_slice=[0, 1])},
+            {("I", 1), ("D", 0), ("G", 1), ("I", 0), ("G", 0), ("D", 1)},
+        )
+        self.assertEqual(
+            set(self.network.get_cpds(time_slice=0)),
             set([self.diff_cpd, self.intel_cpd, self.grade_cpd]),
         )
         self.assertEqual(
             {cpd.variable for cpd in self.network.get_cpds(time_slice=1)},
             {("D", 1), ("I", 1), ("G", 1)},
         )
+        self.assertRaises(ValueError, self.network.get_cpds, time_slice=-1)
+        self.assertRaises(ValueError, self.network.get_cpds, time_slice=[0, 1.1])
+        self.assertRaises(ValueError, self.network.get_cpds, time_slice="abc")
 
     def test_add_multiple_cpds(self):
         self.network.add_edges_from(
diff --git a/pgmpy/tests/test_sampling/test_Sampling.py b/pgmpy/tests/test_sampling/test_Sampling.py
index 5e3304bb..9197090f 100644
--- a/pgmpy/tests/test_sampling/test_Sampling.py
+++ b/pgmpy/tests/test_sampling/test_Sampling.py
@@ -5,6 +5,7 @@ from mock import MagicMock, patch
 from pgmpy.factors.discrete import DiscreteFactor, TabularCPD, State
 from pgmpy.models import BayesianNetwork, MarkovNetwork
 from pgmpy.sampling import BayesianModelSampling, GibbsSampling
+from pgmpy.inference import VariableElimination
 
 
 class TestBayesianModelSampling(unittest.TestCase):
@@ -25,6 +26,10 @@ class TestBayesianModelSampling(unittest.TestCase):
         cpd_g = TabularCPD("G", 2, [[0.6], [0.4]])
         self.bayesian_model.add_cpds(cpd_a, cpd_g, cpd_j, cpd_l, cpd_q, cpd_r)
         self.sampling_inference = BayesianModelSampling(self.bayesian_model)
+        self.forward_marginals = VariableElimination(self.bayesian_model).query(
+            self.bayesian_model.nodes(), joint=False
+        )
+
         # Bayesian Model without state names and with latent variables
         self.bayesian_model_lat = BayesianNetwork(
             [("A", "J"), ("R", "J"), ("J", "Q"), ("J", "L"), ("G", "L")],
@@ -140,8 +145,8 @@ class TestBayesianModelSampling(unittest.TestCase):
 
     def test_forward_sample(self):
         # Test without state names
-        sample = self.sampling_inference.forward_sample(25)
-        self.assertEqual(len(sample), 25)
+        sample = self.sampling_inference.forward_sample(int(1e5))
+        self.assertEqual(len(sample), int(1e5))
         self.assertEqual(len(sample.columns), 6)
         self.assertIn("A", sample.columns)
         self.assertIn("J", sample.columns)
@@ -156,6 +161,19 @@ class TestBayesianModelSampling(unittest.TestCase):
         self.assertTrue(set(sample.G).issubset({0, 1}))
         self.assertTrue(set(sample.L).issubset({0, 1}))
 
+        # Test that the marginal distribution of samples is same as the model
+        sample_marginals = {
+            node: sample[node].value_counts() / sample.shape[0]
+            for node in self.bayesian_model.nodes()
+        }
+
+        for node in self.bayesian_model.nodes():
+            for state in [0, 1]:
+                self.assertEqual(
+                    round(self.forward_marginals[node].get_value(**{node: state}), 1),
+                    round(sample_marginals[node].loc[state], 1),
+                )
+
         # Test without state names and with latents
         sample = self.sampling_inference_lat.forward_sample(25, include_latents=True)
         self.assertEqual(len(sample), 25)
@@ -217,9 +235,9 @@ class TestBayesianModelSampling(unittest.TestCase):
         # Test without state names
         sample = self.sampling_inference.rejection_sample()
         sample = self.sampling_inference.rejection_sample(
-            [State("A", 1), State("J", 1), State("R", 1)], 25
+            [State("A", 1), State("J", 1), State("R", 1)], int(1e5)
         )
-        self.assertEqual(len(sample), 25)
+        self.assertEqual(len(sample), int(1e5))
         self.assertEqual(len(sample.columns), 6)
         self.assertEqual(set(sample.columns), {"A", "J", "R", "Q", "G", "L"})
         self.assertTrue(set(sample.A).issubset({1}))
@@ -229,6 +247,23 @@ class TestBayesianModelSampling(unittest.TestCase):
         self.assertTrue(set(sample.G).issubset({0, 1}))
         self.assertTrue(set(sample.L).issubset({0, 1}))
 
+        # Test that the marginal distributions is the same in model and samples
+        self.rejection_marginals = VariableElimination(self.bayesian_model).query(
+            ["Q", "G", "L"], evidence={"A": 1, "J": 1, "R": 1}, joint=False
+        )
+
+        sample_marginals = {
+            node: sample[node].value_counts() / sample.shape[0]
+            for node in ["Q", "G", "L"]
+        }
+
+        for node in ["Q", "G", "L"]:
+            for state in [0, 1]:
+                self.assertEqual(
+                    round(self.rejection_marginals[node].get_value(**{node: state}), 1),
+                    round(sample_marginals[node].loc[state], 1),
+                )
+
         # Test without state names with latent variables
         sample = self.sampling_inference_lat.rejection_sample(
             [State("A", 1), State("J", 1), State("R", 1)], 25, include_latents=True