DynamicBayesianNetwork.py: Added get_cardinality method #924

lohani2280 · 2017-10-27T18:41:15Z

closes #921

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
Make sure you are making a pull request against the dev branch (left side). Also you should start your branch off our dev.
Check the commit's or even all commits' message styles matches our requested structure.
Check your code additions will fail neither code linting checks nor unit test.

Fixes #921

Changes

Added get_cardinality method in DynamicBayesianNetwork class and the corresponding test cases.

💔Thank you!

ankurankan · 2017-11-02T13:20:42Z

pgmpy/models/DynamicBayesianNetwork.py

+            if node not in super(DynamicBayesianNetwork, self).nodes():
+                raise ValueError('Node not present in the model.')
+            else:
+                temp_node = (node[0], 1 - node[1]) if node[1] else node


@lohani2280 I am unable to understand why you are using 1 - node[1]. Could you please explain a bit ?

@ankurankan

>>> 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.8,0.03], ... [0.3,0.7,0.02,0.2]], [('I', 0),('D', 0)],[2,2]) >>> dbn.add_cpds(grade_cpd)

In DBN the cardinality of the node will be same at every time slice. Now, If the node queried using get_cardinality is in time slice = 0 then temp_node = node. However, if the queried node is not in time slice 0 then using 1-node[1] ,i brought it to time slice 0 and then proceed to output the cardinality of the queired node at time slice =0.

For example -
If dbn.get_cardinality(('D',1)) is called then -
node = ('D',1)
node[1] = 1
therefore, temp_node = (node[0],1 - node[1]) = ('D',0)

Similarly if dbn.get_cardinality(('D',0)) is called then -
node = ('D',0)
node[1] = 0
therefore, temp_node = node = ('D',0).

Actually , with this i am thinking to replace

pgmpy/pgmpy/models/DynamicBayesianNetwork.py

Line 512 in 0f62927

evidence_card = cpd.cardinality[:0:-1]

with following piece of code to solve issue #877

parents_card = [] for parent in parents: parents_card.append(self.get_cardinality(parent))

@lohani2280 But what if someone queries in let's say time slice 5, with the node ('D', 5). This would fail in that case. Right ?

@ankurankan Ohh!! sorry my bad. I went out with the flow of the example in the docstring.

khalibartan · 2017-11-05T11:30:18Z

@ankurankan @lohani2280 If I remember correctly it is not necessary for a node to be present in time slice 0 always.

ankurankan · 2017-11-05T12:01:33Z

pgmpy/models/DynamicBayesianNetwork.py

+            if node not in super(DynamicBayesianNetwork, self).nodes():
+                raise ValueError('Node not present in the model.')
+            else:
+                temp_node = (node[0],0) if node[1] else node


@lohani2280 PEP8. Space after ,.

ankurankan · 2017-11-05T12:02:21Z

pgmpy/models/DynamicBayesianNetwork.py

+        >>> 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],


Always use keyword argument in docstrings. Makes things clearer for the user.

ankurankan · 2017-11-05T12:03:56Z

pgmpy/tests/test_models/test_DynamicBayesianNetwork.py

+        self.assertDictEqual(self.network.get_cardinality(),
+                             {('D', 0): 2, ('D', 1): 2, ('G', 0): 3, ('I', 0): 2, ('I', 1): 2})
+
+    def test_get_cardinality_with_node(self):


@lohani2280 Could you also add tests for the case when the time slice is not 0 or 1.

ankurankan · 2017-11-05T12:08:38Z

pgmpy/models/DynamicBayesianNetwork.py

+        defaultdict(int, {('D', 0): 2, ('D', 1): 2, ('G', 0): 3, ('I', 0): 2, ('I', 1): 2})
+        """
+        if node:
+            if node not in super(DynamicBayesianNetwork, self).nodes():


I think this will work only when the time slice is 0 or 1. Also add tests for testing when this method will throw this error.

ankurankan · 2017-11-05T12:17:46Z

pgmpy/models/DynamicBayesianNetwork.py

@@ -440,6 +440,51 @@ def remove_cpds(self, *cpds):
                cpd = self.get_cpds(cpd)
            self.cpds.remove(cpd)

+    def get_cardinality(self, node=None):
+        """
+        Returns the cardinality of the node.


I think we should also mention here that it returns all the cardinalities if node=None.

ankurankan · 2017-11-05T12:18:33Z

@khalibartan Yes, you are right. I mentioned that only in the last comment.

lohani2280 · 2017-11-05T12:22:59Z

@ankurankan @khalibartan Thanks for the review. I will make the suggested changes and synchronise soon :)

codecov · 2017-12-13T09:05:24Z

Codecov Report

Merging #924 into dev will increase coverage by <.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##              dev     #924      +/-   ##
==========================================
+ Coverage   94.77%   94.77%   +<.01%     
==========================================
  Files         116      116              
  Lines       11314    11336      +22     
==========================================
+ Hits        10723    10744      +21     
- Misses        591      592       +1

Impacted Files	Coverage Δ
...y/tests/test_models/test_DynamicBayesianNetwork.py	`100% <100%> (ø)`	⬆️
pgmpy/models/DynamicBayesianNetwork.py	`91.66% <90%> (-0.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e45f746...a95e7f5. Read the comment docs.

lohani2280 · 2017-12-13T09:17:18Z

@ankurankan I have updated the PR with the suggested changes. Please have a look!

ankurankan · 2017-12-20T17:54:55Z

pgmpy/models/DynamicBayesianNetwork.py

+        """
+        Returns the cardinality of the node.If node=None returns all the cardinalities.
+
+        Parameter


@lohani2280 Use Parameters, the doc generator parses that only I think.

@ankurankan Yeah sure, i'll update it.

lohani2280 · 2018-01-17T13:18:12Z

@ankurankan I think this PR is ready for merge. I have rebased it. If you are free at sometime , please have a look over it.

khalibartan · 2018-03-11T14:54:41Z

@lohani2280 Can you rebase your branch to latest dev?

closes pgmpy#921

lohani2280 · 2018-03-12T20:36:53Z

@khalibartan @ankurankan I have rebased my branch to latest dev. Please have a look at it.

Ayush has updated the PR

khalibartan

@lohani2280 Could you please add test cases of error that is being raised.

khalibartan · 2018-03-13T10:13:39Z

pgmpy/models/DynamicBayesianNetwork.py

+            if node not in super(DynamicBayesianNetwork, self).nodes():
+                raise ValueError('Node not present in the model.')
+            else:
+                temp_node = (node[0], 0) if node[1] else node


@lohani2280 I don't think we need this line. You can directly write something like this
return self.get_cpds((node[0], 0)).cardinality[0]

@khalibartan Sorry, for the late reply. I'll update the PR very soon.

ankurankan reviewed Nov 2, 2017

View reviewed changes

lohani2280 force-pushed the DBN_branch_2 branch from 33fe43a to 59e80ba Compare November 5, 2017 09:14

ankurankan requested changes Nov 5, 2017

View reviewed changes

lohani2280 force-pushed the DBN_branch_2 branch from 59e80ba to fe4b10f Compare December 13, 2017 09:05

lohani2280 closed this Dec 13, 2017

lohani2280 reopened this Dec 13, 2017

ankurankan previously requested changes Dec 20, 2017

View reviewed changes

lohani2280 force-pushed the DBN_branch_2 branch from fe4b10f to a3be71f Compare December 20, 2017 18:03

lohani2280 force-pushed the DBN_branch_2 branch from a3be71f to 2ac86b0 Compare January 17, 2018 13:17

DynamicBayesianNetwork.py: Added get_cardinality method

a95e7f5

closes pgmpy#921

lohani2280 force-pushed the DBN_branch_2 branch from 2ac86b0 to a95e7f5 Compare March 12, 2018 20:14

khalibartan approved these changes Mar 13, 2018

View reviewed changes

khalibartan suggested changes Mar 13, 2018

View reviewed changes

khalibartan reviewed Mar 13, 2018

View reviewed changes

ankurankan self-assigned this Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DynamicBayesianNetwork.py: Added get_cardinality method #924

DynamicBayesianNetwork.py: Added get_cardinality method #924

lohani2280 commented Oct 27, 2017 •

edited

ankurankan Nov 2, 2017

lohani2280 Nov 4, 2017

ankurankan Nov 4, 2017

lohani2280 Nov 4, 2017

khalibartan commented Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan commented Nov 5, 2017

lohani2280 commented Nov 5, 2017

codecov bot commented Dec 13, 2017 •

edited

lohani2280 commented Dec 13, 2017

ankurankan Dec 20, 2017

lohani2280 Dec 20, 2017

lohani2280 commented Jan 17, 2018

khalibartan commented Mar 11, 2018

lohani2280 commented Mar 12, 2018

khalibartan left a comment •

edited

khalibartan Mar 13, 2018 •

edited

lohani2280 Apr 1, 2018

DynamicBayesianNetwork.py: Added get_cardinality method #924

Are you sure you want to change the base?

DynamicBayesianNetwork.py: Added get_cardinality method #924

Conversation

lohani2280 commented Oct 27, 2017 • edited

Your checklist for this pull request

Fixes #921

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khalibartan commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankurankan commented Nov 5, 2017

lohani2280 commented Nov 5, 2017

codecov bot commented Dec 13, 2017 • edited

Codecov Report

lohani2280 commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lohani2280 commented Jan 17, 2018

khalibartan commented Mar 11, 2018

lohani2280 commented Mar 12, 2018

khalibartan left a comment • edited

Choose a reason for hiding this comment

khalibartan Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lohani2280 commented Oct 27, 2017 •

edited

codecov bot commented Dec 13, 2017 •

edited

khalibartan left a comment •

edited

khalibartan Mar 13, 2018 •

edited