Change in MarkovModel #1002

VSSILPA · 2018-03-30T09:23:57Z

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.

Changes

Changed factor_node to factor in line number 295 and added the corresponding test cases.

💔Thank you!

khalibartan · 2018-03-30T09:53:11Z

@ankurankan I'm not in favour of the current factor graph tests. Tests are using both factor or str phi based on convenience in different places.

VSSILPA · 2018-03-30T09:58:20Z

Yes, the tests are failing. In some places, factor nodes are taken as string while in the other cases, it's of dicrete factor class.

khalibartan · 2018-03-30T09:56:28Z

pgmpy/tests/test_models/test_MarkovModel.py

-        self.assertListEqual(hf.recursive_sorted(factor_graph.edges()),
-                             [['Alice', 'phi_Alice_Bob'], ['Bob', 'phi_Alice_Bob'],
-                              ['Bob', 'phi_Bob_Charles'], ['Charles', 'phi_Bob_Charles']])
+        self.assertListEqual(factor_graph.nodes(),


@VSSILPA You can convert both lists to python set. Order of elements might change in list, so test might fail or pass.

khalibartan · 2018-03-30T10:01:14Z

pgmpy/tests/test_models/test_MarkovModel.py

+                             ['Alice', 'Bob', 'Charles', phi1,
+                              phi2])
+        self.assertListEqual(factor_graph.edges(),
+                             [['Alice', phi1], ['Bob', phi1],


@VSSILPA same goes for this. Convert both of them to set. Also, note that the edge will be a tuple and not list

khalibartan · 2018-03-30T10:57:30Z

pgmpy/tests/test_models/test_MarkovModel.py

+                            set(['Alice', 'Bob', 'Charles', phi1,
+                              phi2]))
+        self.assertSetEqual(set(factor_graph.edges()),
+                            set([('Alice', phi1), ('Bob', phi1),


@VSSILPA Correct order would be:
set([('Bob', phi1), ('Bob', phi2), ('Charles', phi2), (phi1, 'Alice')])

You can either change it to above or you can convert the tuples to frozenset in both cases. I'd like to go with latter one, add this function in help_function.py in test directory.

I've converted tuples to frozenset. Is this fine?

It's working for all test cases now.

codecov · 2018-03-30T12:06:06Z

Codecov Report

Merging #1002 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev    #1002      +/-   ##
==========================================
+ Coverage   94.77%   94.77%   +<.01%     
==========================================
  Files         116      116              
  Lines       11314    11317       +3     
==========================================
+ Hits        10723    10726       +3     
  Misses        591      591

Impacted Files	Coverage Δ
pgmpy/models/MarkovModel.py	`97.3% <100%> (-0.02%)`	⬇️
pgmpy/tests/help_functions.py	`100% <100%> (ø)`	⬆️
pgmpy/tests/test_models/test_MarkovModel.py	`100% <100%> (ø)`	⬆️

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 f9be170...e5b4cb1. Read the comment docs.

khalibartan

Please squash your commits.

khalibartan · 2018-03-30T14:24:27Z

pgmpy/tests/test_models/test_MarkovModel.py

@@ -215,13 +215,16 @@ def test_factor_graph(self):
        self.graph.add_factors(phi1, phi2)

        factor_graph = self.graph.to_factor_graph()
+


@VSSILPA Remove these empty lines. Also, can you check for pep8 errors in your code? You can use
pycodestyle --max-line-length=120 <filename> for this check.

khalibartan · 2018-03-30T14:41:05Z

pgmpy/tests/help_functions.py

@@ -5,3 +5,8 @@ def recursive_sorted(li):
    for i in range(len(li)):
        li[i] = sorted(li[i])
    return sorted(li)
+
+def convert_to_frozenset(edges) :


@VSSILPA I think we can have a better name for this function. Also IMO it should return a frozenset of frozenset.
So function name could be recursive_frozenset, if you can think of some name, please use that.

khalibartan · 2018-03-30T14:42:29Z

pgmpy/models/MarkovModel.py

@@ -291,8 +291,8 @@ def to_factor_graph(self):
        factor_graph.add_nodes_from(self.nodes())
        for factor in self.factors:
            scope = factor.scope()
-            factor_node = 'phi_' + '_'.join(scope)
-            factor_graph.add_edges_from(itertools.product(scope, [factor_node]))
+#            factor_node = 'phi_' + '_'.join(scope)


@VSSILPA Remove this commented line.

changed test_cases_Markov changed test_cases_Markov changed test_cases_Markov changed revised_test_cases_MarkovModel final_change

ankurankan · 2018-03-31T12:10:46Z

@VSSILPA I am not sure about this change. Using a factor object as a node might break the consistency in API for inference or learning. And that will make us add extra conditions for handling it all the higher level APIs.

khalibartan · 2018-04-12T05:41:17Z

@ankurankan I think we should discuss upon the design of FactorGraph class. Currently, in FactorGraph class factor nodes are actual factors and that too isn't being tested. There is high inconsistency in implementation. We can discuss the design and then finalize how we want to proceed. i do too agree with the fact that adding factor as node will cause difficulty.

khalibartan suggested changes Mar 30, 2018

View reviewed changes

final_change

e5b4cb1

changed test_cases_Markov changed test_cases_Markov changed test_cases_Markov changed revised_test_cases_MarkovModel final_change

VSSILPA force-pushed the changeMarkovModel branch from f7bf3e4 to e5b4cb1 Compare March 30, 2018 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change in MarkovModel #1002

Change in MarkovModel #1002

VSSILPA commented Mar 30, 2018 •

edited

khalibartan commented Mar 30, 2018

VSSILPA commented Mar 30, 2018

khalibartan Mar 30, 2018 •

edited

khalibartan Mar 30, 2018 •

edited

khalibartan Mar 30, 2018

VSSILPA Mar 30, 2018

VSSILPA Mar 30, 2018

codecov bot commented Mar 30, 2018 •

edited

khalibartan left a comment

khalibartan Mar 30, 2018

khalibartan Mar 30, 2018

khalibartan Mar 30, 2018

ankurankan commented Mar 31, 2018

khalibartan commented Apr 12, 2018

		@@ -215,13 +215,16 @@ def test_factor_graph(self):
		self.graph.add_factors(phi1, phi2)

		factor_graph = self.graph.to_factor_graph()

Change in MarkovModel #1002

Are you sure you want to change the base?

Change in MarkovModel #1002

Conversation

VSSILPA commented Mar 30, 2018 • edited

Your checklist for this pull request

Changes

khalibartan commented Mar 30, 2018

VSSILPA commented Mar 30, 2018

khalibartan Mar 30, 2018 • edited

Choose a reason for hiding this comment

khalibartan Mar 30, 2018 • edited

Choose a reason for hiding this comment

khalibartan Mar 30, 2018

Choose a reason for hiding this comment

VSSILPA Mar 30, 2018

Choose a reason for hiding this comment

VSSILPA Mar 30, 2018

Choose a reason for hiding this comment

codecov bot commented Mar 30, 2018 • edited

Codecov Report

khalibartan left a comment

Choose a reason for hiding this comment

khalibartan Mar 30, 2018

Choose a reason for hiding this comment

khalibartan Mar 30, 2018

Choose a reason for hiding this comment

khalibartan Mar 30, 2018

Choose a reason for hiding this comment

ankurankan commented Mar 31, 2018

khalibartan commented Apr 12, 2018

VSSILPA commented Mar 30, 2018 •

edited

khalibartan Mar 30, 2018 •

edited

khalibartan Mar 30, 2018 •

edited

codecov bot commented Mar 30, 2018 •

edited