Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932

lohani2280 · 2017-11-05T12:08:37Z

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 #683

Changes

Please list the proposed changes in this pull request.

Removed the inconsistent behavior of query method in Variable Elimination.Now the query method
returns an empty dict for no variables specified
FactorGraph.py: Fixed a small typo error in docstring of get_cardinality method.
DynamicBayesianNetwork.py: Fixed small typo error in the docstring.

💔Thank you!

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

pgmpy/inference/ExactInference.py

-            for factor_li in self.factors.values():
-                all_factors.extend(factor_li)
-            return set(all_factors)
+            return {}


@lohani2280 I can't understand this change. Elaborate please ? And I think the tests are failing because of this.

Sorry, my bad. Hadn't read the whole issue.

No changes needed

ankurankan

Tests should be easy to fix. Just pass in all the variables instead of None to the query or map_query methods of the failing tests.

…tion Now the query method returns an empty dict for no variables specified closes pgmpy#683

…ity method.

Evidence cardinality must be a list of numbers.This was missing at some places in DynamicBayesianNetwork.

lohani2280 · 2017-11-06T14:49:18Z

@ankurankan Edited the PR as suggested. Please review it.

codecov · 2017-11-06T14:52:55Z

Codecov Report

Merging #932 into dev will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##              dev    #932      +/-   ##
=========================================
- Coverage   94.71%   94.7%   -0.02%     
=========================================
  Files         114     114              
  Lines       11176   11173       -3     
=========================================
- Hits        10585   10581       -4     
- Misses        591     592       +1

Impacted Files	Coverage Δ
pgmpy/models/FactorGraph.py	`94.94% <ø> (ø)`	⬆️
pgmpy/models/DynamicBayesianNetwork.py	`91.8% <ø> (ø)`	⬆️
pgmpy/inference/ExactInference.py	`93.3% <0%> (-1.51%)`	⬇️
pgmpy/tests/test_inference/test_ExactInference.py	`100% <100%> (ø)`	⬆️
pgmpy/models/BayesianModel.py	`95.88% <0%> (+0.82%)`	⬆️

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 110b4c1...534f851. Read the comment docs.

ankurankan · 2017-11-06T16:55:30Z

pgmpy/tests/test_inference/test_ExactInference.py

@@ -93,7 +93,7 @@ def test_query_multiple_times(self):
                                          np.array([0.772727, 0.227273]))

    def test_max_marginal(self):
-        np_test.assert_almost_equal(self.bayesian_inference.max_marginal(), 0.1659, decimal=4)
+        np_test.assert_almost_equal(self.bayesian_inference.max_marginal(['A','R','J','Q','G','L']), 0.0516, decimal=4)


@lohani2280 Any idea why these values are changing ? And should they ?

@ankurankan With _variable_elimination method we are implementing variable elimination in a generalised way.Many other methods are using this method internally. So, any change here will affect other methods too.
In our issue we are only concerned with inconsistency with the query method.We just want an empty dict to be returned if no variables is specified.I think implementing it locally in the query method would be the best idea. I would synchronise the PR soon. Please suggest your views.

@lohani2280 I don't think implementing it locally would be a good idea because we have other methods which use _variable_elimination and we would like to have the behavior consistent across all these methods. Plus it will add boilerplate code. Could you just check once if the values returned now are correct or the previous values were correct ?

@ankurankan The values are actually not changing. If we run the query bayesian_inference.max_marginal(['A','J','R','Q','L','G']) , either on this PR or without this, value is same, i.e, 0.0516. Its just that if we run the query bayesian_inference.max_marginal() on this PR it throws an error because this method call for _variable_elimination for which we have just applied, that it should return an empty dict on passing an empty list of variables.

@lohani2280 Okay. But isn't calling max_marginal() in the previous version and calling max_marginal(['A', 'J', 'R', 'Q', 'L', 'G']) in this PR equivalent ? I mean that in the previous version, didn't passing no argument meant computing max_marginal over all the variables in the network ? Because if that's the case the value for your new query should still be 0.1659. Right ?

@ankurankan Right. Let me look a bit more deeply and then i'll get back again.

@ankurankan Calling max_marginal() in the previous version meant computing max_marginal over all the variables in the network. Also, calling max_marginal(['A', 'J', 'R', 'Q', 'L', 'G']) in the previous version meant computing max_marginal over all the variables in the network.
In the previous version if we run the query bayesian_inference.max_marginal() , we get 0.1659. However for bayesian_inference.max_marginal(['A', 'J', 'R', 'Q', 'L', 'G']) we get 0.0516. Ideally, both the query should have same answer.Thus,there is an ambiguity. IMO value of max_marginal() is correct and max_marginal(['A', 'J', 'R', 'Q', 'L', 'G']) isn't.
I think i found the bug --
When we query max_marginal(['A', 'J', 'R', 'Q', 'L', 'G']), _variable_elimination should have returned

pgmpy/pgmpy/inference/ExactInference.py

Line 90 in fe2ca2c

final_distribution.add(factor)

instead of

pgmpy/pgmpy/inference/ExactInference.py

Line 98 in fe2ca2c

return query_var_factor

.
Please suggest your opinion.

ankurankan previously requested changes Nov 5, 2017

View reviewed changes

ankurankan requested changes Nov 5, 2017

View reviewed changes

lohani2280 added 3 commits November 6, 2017 20:05

Removed the inconsistent behavior of query method in Variable Elimina…

43dff10

…tion Now the query method returns an empty dict for no variables specified closes pgmpy#683

FactorGraph.py: Fixed a small typo error in docstring of get_cardinal…

942f0b7

…ity method.

DynamicBayesianNetwork.py: Fixed small typo error in the docstring.

534f851

Evidence cardinality must be a list of numbers.This was missing at some places in DynamicBayesianNetwork.

lohani2280 force-pushed the inference/query/docstring branch from 97e0ee8 to 534f851 Compare November 6, 2017 14:41

ankurankan reviewed Nov 6, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932

Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932

lohani2280 commented Nov 5, 2017 •

edited

ankurankan Nov 5, 2017

ankurankan Nov 5, 2017

ankurankan left a comment

lohani2280 commented Nov 6, 2017

codecov bot commented Nov 6, 2017 •

edited

ankurankan Nov 6, 2017

lohani2280 Nov 8, 2017

ankurankan Nov 8, 2017

lohani2280 Dec 16, 2017 •

edited

ankurankan Dec 18, 2017

lohani2280 Dec 19, 2017

lohani2280 Dec 21, 2017 •

edited

Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932

Are you sure you want to change the base?

Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932

Conversation

lohani2280 commented Nov 5, 2017 • edited

Your checklist for this pull request

Fixes #683

Changes

ankurankan Nov 5, 2017

Choose a reason for hiding this comment

ankurankan Nov 5, 2017

Choose a reason for hiding this comment

ankurankan left a comment

Choose a reason for hiding this comment

lohani2280 commented Nov 6, 2017

codecov bot commented Nov 6, 2017 • edited

Codecov Report

ankurankan Nov 6, 2017

Choose a reason for hiding this comment

lohani2280 Nov 8, 2017

Choose a reason for hiding this comment

ankurankan Nov 8, 2017

Choose a reason for hiding this comment

lohani2280 Dec 16, 2017 • edited

Choose a reason for hiding this comment

ankurankan Dec 18, 2017

Choose a reason for hiding this comment

lohani2280 Dec 19, 2017

Choose a reason for hiding this comment

lohani2280 Dec 21, 2017 • edited

Choose a reason for hiding this comment

lohani2280 commented Nov 5, 2017 •

edited

codecov bot commented Nov 6, 2017 •

edited

lohani2280 Dec 16, 2017 •

edited

lohani2280 Dec 21, 2017 •

edited