-
Notifications
You must be signed in to change notification settings - Fork 699
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed the inconsistent behavior of query method in Variable Elimination and fixed small typo errors #932
base: dev
Are you sure you want to change the base?
Conversation
for factor_li in self.factors.values(): | ||
all_factors.extend(factor_li) | ||
return set(all_factors) | ||
return {} |
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.
@lohani2280 I can't understand this change. Elaborate please ? And I think the tests are failing because of this.
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.
Sorry, my bad. Hadn't read the whole issue.
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.
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
Evidence cardinality must be a list of numbers.This was missing at some places in DynamicBayesianNetwork.
97e0ee8
to
534f851
Compare
@ankurankan Edited the PR as suggested. Please review it. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -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) |
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.
@lohani2280 Any idea why these values are changing ? And should they ?
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.
@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.
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.
@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 ?
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.
@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.
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.
@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 ?
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.
@ankurankan Right. Let me look a bit more deeply and then i'll get back again.
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.
@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.
Your checklist for this pull request
🚨Please review the guidelines for contributing to this repository.
Fixes #683
Changes
Please list the proposed changes in this pull request.
returns an empty dict for no variables specified
💔Thank you!