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
Merged

Added minimal python3.5 test env #3470

merged 12 commits into from May 21, 2019

Conversation

lucianopaz
Copy link
Contributor

After #3465 and taking into account that we want to keep a tight integration with Arviz, I think that we should add a small test environment for python 3.5. I added test_sampling.py and test_model_graph.py because we introduced type hints into sampling.py and model_graph.py, and we need to check if those work well or break python 3.5.

.travis.yml Outdated
@@ -30,6 +30,8 @@ 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"
- FLOATX='float64' TESTCMD="--durations=10 --cov-append pymc3/tests/test_variational_inference.py pymc3/tests/test_updates.py pymc3/tests/test_shape_handling.py"
Copy link
Member

Choose a reason for hiding this comment

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

duplicate to above line.

@lucianopaz
Copy link
Contributor Author

I think that this is finally ready for review

@@ -46,3 +46,4 @@ 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
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

# 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

@canyon289
Copy link
Member

A far as I can tell LGTM. I could use help reviewing the draws_values portion

@lucianopaz
Copy link
Contributor Author

Should we add a python version requirement now?

@canyon289
Copy link
Member

canyon289 commented May 10, 2019

I think so, but once we do this we should cut a release, not to break existing environments

@rpgoldman
Copy link
Contributor

I think so, but once we do this we should cut a release, not to break existing environments

If you are going to do that, may I get #3468 merged before the new release?

@twiecki twiecki merged commit 940d784 into pymc-devs:master May 21, 2019
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

4 participants