-
Notifications
You must be signed in to change notification settings - Fork 692
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change in MarkovModel #1002
base: dev
Are you sure you want to change the base?
Change in MarkovModel #1002
Conversation
@ankurankan I'm not in favour of the current factor graph tests. Tests are using both factor or |
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. |
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VSSILPA You can convert both lists to python set. Order of elements might change in list, so test might fail or pass.
['Alice', 'Bob', 'Charles', phi1, | ||
phi2]) | ||
self.assertListEqual(factor_graph.edges(), | ||
[['Alice', phi1], ['Bob', phi1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VSSILPA same goes for this. Convert both of them to set. Also, note that the edge will be a tuple and not list
set(['Alice', 'Bob', 'Charles', phi1, | ||
phi2])) | ||
self.assertSetEqual(set(factor_graph.edges()), | ||
set([('Alice', phi1), ('Bob', phi1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've converted tuples to frozenset. Is this fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working for all test cases now.
Codecov Report
@@ Coverage Diff @@
## dev #1002 +/- ##
==========================================
+ Coverage 94.77% 94.77% +<.01%
==========================================
Files 116 116
Lines 11314 11317 +3
==========================================
+ Hits 10723 10726 +3
Misses 591 591
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash your commits.
@@ -215,13 +215,16 @@ def test_factor_graph(self): | |||
self.graph.add_factors(phi1, phi2) | |||
|
|||
factor_graph = self.graph.to_factor_graph() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
pgmpy/tests/help_functions.py
Outdated
@@ -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) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
pgmpy/models/MarkovModel.py
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VSSILPA Remove this commented line.
changed test_cases_Markov changed test_cases_Markov changed test_cases_Markov changed revised_test_cases_MarkovModel final_change
f7bf3e4
to
e5b4cb1
Compare
@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. |
@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. |
Your checklist for this pull request
馃毃Please review the guidelines for contributing to this repository.
Changes
Changed factor_node to factor in line number 295 and added the corresponding test cases.
馃挃Thank you!