Skip to content
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

Bug fixes for ExactInference on continuous factors #941

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

linzhp
Copy link
Contributor

@linzhp linzhp commented Nov 8, 2017

Variable Elimination for continuous variables was broken when there is only one query variable, as illustrated by the added unit test in this PR. This PR fixed it.

@ankurankan Please review.

@ankurankan
Copy link
Member

@linzhp The tests are failing, could you check please ?

@linzhp
Copy link
Contributor Author

linzhp commented Nov 9, 2017

I tried Python 3.6, 3.5 and 2.7 on Windows, but I can't reproduce the test failure. I will try running it on Linux.

Also, from the Travis' log, I saw this:

Setting environment variables from .travis.yml
$ export DISTRIB="conda"
$ export PYTHON_VERSION="3.5"
$ export COVERAGE="true"
$ source ~/virtualenv/python2.7/bin/activate
$ python --version
Python 2.7.13

The Python version it used was wrong. So essentially, Travis only ran the test in Python 2.7 for three times.

@ankurankan
Copy link
Member

Yeah, it's not failing on windows. The appveyor builds on windows and it's passing.

The Python version it used was wrong. So essentially, Travis only ran the test in Python 2.7 for three times.

No, the python versions are correct actually. The python --version is ran before we setup our conda environment, so it's basically the default python version on the system. I will actually change this logging, it's a bit confusing.

@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #941 into dev will increase coverage by 0.05%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #941      +/-   ##
==========================================
+ Coverage   94.71%   94.76%   +0.05%     
==========================================
  Files         114      114              
  Lines       11185    11201      +16     
==========================================
+ Hits        10594    10615      +21     
+ Misses        591      586       -5
Impacted Files Coverage Δ
pgmpy/factors/continuous/ContinuousFactor.py 91.78% <0%> (+5.47%) ⬆️
pgmpy/inference/ExactInference.py 94.81% <100%> (ø) ⬆️
pgmpy/tests/test_inference/test_ExactInference.py 100% <100%> (ø) ⬆️
pgmpy/factors/distributions/CustomDistribution.py 96.72% <90%> (+1.1%) ⬆️

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 5953dcd...556f87a. Read the comment docs.

@linzhp
Copy link
Contributor Author

linzhp commented Nov 10, 2017

All tests passed now. Please take another look, @ankurankan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants