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

Added minimal python3.5 test env #3470

Merged
merged 12 commits into from May 21, 2019
1 change: 1 addition & 0 deletions .travis.yml
Expand Up @@ -30,6 +30,7 @@ env:
- FLOATX='float64' TESTCMD="--durations=10 --cov-append pymc3/tests/test_distributions_random.py pymc3/tests/test_shared.py pymc3/tests/test_smc.py pymc3/tests/test_sampling.py pymc3/tests/test_parallel_sampling.py pymc3/tests/test_dist_math.py pymc3/tests/test_distribution_defaults.py pymc3/tests/test_distributions_timeseries.py pymc3/tests/test_random.py"
- FLOATX='float64' TESTCMD="--durations=10 --cov-append pymc3/tests/test_examples.py pymc3/tests/test_posteriors.py pymc3/tests/test_gp.py"
- FLOATX='float64' TESTCMD="--durations=10 --cov-append pymc3/tests/test_variational_inference.py pymc3/tests/test_updates.py pymc3/tests/test_shape_handling.py"
- PYTHON_VERSION=3.5 FLOATX='float64' ENVNAME="py35_testenv" TESTCMD="--cov-append pymc3/tests/test_sampling.py pymc3/tests/test_model_graph.py"

script:
- . ./scripts/test.sh $TESTCMD
Expand Down
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
Expand Up @@ -45,6 +45,7 @@
- Fixed an issue in `model_graph` that caused construction of the graph of the model for rendering to hang: replaced a search over the powerset of the nodes with a breadth-first search over the nodes. Fix for #3458.
- Removed variable annotations from `model_graph` but left type hints (Fix for #3465). This means that we support `python>=3.5.4`.
- Default `target_accept`for `HamiltonianMC` is now 0.65, as suggested in Beskos et. al. 2010 and Neal 2001.
- Fixed bug in `draw_values` that lead to intermittent errors in python3.5. This happened with some deterministic nodes that were drawn but not added to `givens`.

### Deprecations

Expand Down
17 changes: 15 additions & 2 deletions pymc3/distributions/distribution.py
Expand Up @@ -401,8 +401,7 @@ def draw_values(params, point=None, size=None):
# nodes in the `params` list.
stack.extend([node for node in named_nodes_parents[next_]
if node is not None and
(node, size) not in drawn and
node not in params])
(node, size) not in drawn])

# the below makes sure the graph is evaluated in order
# test_distributions_random::TestDrawValues::test_draw_order fails without it
Expand All @@ -420,6 +419,20 @@ def draw_values(params, point=None, size=None):
evaluated[param_idx] = drawn[(param, size)]
else:
try: # might evaluate in a bad order,
# Sometimes _draw_value recurrently calls draw_values.
# This may set values for certain nodes in the drawn
# dictionary, but they don't get added to the givens
# dictionary. Here, we try to fix that.
if param in named_nodes_children:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my cursory knowledge this looks good! Out of curiosity though why would the bug only present itself in one version of python?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was super puzzled by this too! I finally found that the problem was caused because python 3.6 preserves insertion order for sets and dictionaries, whereas earlier versions did not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, optional but related. Raymond Hettinger gave an excellent talk on how this came about
https://www.youtube.com/watch?v=npw4s1QTmPg and he was nice enough to answer my question about it on Stackoverflow!

https://stackoverflow.com/questions/44172039/in-raymond-hettingers-pycon-2017-talk-what-is-the-database-representation

for node in named_nodes_children[param]:
if (
node.name not in givens and
(node, size) in drawn
):
givens[node.name] = (
node,
drawn[(node, size)]
)
value = _draw_value(param,
point=point,
givens=givens.values(),
Expand Down
7 changes: 4 additions & 3 deletions scripts/create_testenv.sh
Expand Up @@ -20,7 +20,7 @@ command -v conda >/dev/null 2>&1 || {
exit 1;
}

ENVNAME="testenv"
ENVNAME="${ENVNAME:-testenv}" # if no ENVNAME is specified, use testenv
PYTHON_VERSION=${PYTHON_VERSION:-3.6} # if no python specified, use 3.6

if [ -z ${GLOBAL} ]
Expand All @@ -33,10 +33,11 @@ then
fi
source activate ${ENVNAME}
fi
conda install --yes numpy scipy mkl-service
pip install --upgrade pip

conda install --yes mkl-service
conda install --yes -c conda-forge python-graphviz

pip install --upgrade pip

# Install editable using the setup.py

Expand Down
2 changes: 2 additions & 0 deletions scripts/install_miniconda.sh
Expand Up @@ -46,3 +46,5 @@ fi
export PATH="$INSTALL_FOLDER/bin:$PATH"
echo "Adding $INSTALL_FOLDER to PATH. Consider adding it in your .rc file as well."
conda update -q -y conda
# Uninstalling miniconda's numpy to avoid conflicting versions when creating the test env
pip uninstall -y numpy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a comment that this is installing TravisCI's version of numpy. At a glance its confusing why numpy would be uninstalled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ravin! I'll add it once I get back to my computer in a few hours

1 change: 1 addition & 0 deletions setup.py
Expand Up @@ -59,6 +59,7 @@ def get_version():
package_data={'docs': ['*']},
include_package_data=True,
classifiers=classifiers,
python_requires=">=3.5.4",
install_requires=install_reqs,
extras_require={
"plots": ["arviz"],
Expand Down