Tests: Simple sense test for linear regression #39

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.

Sign up for GitHub

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

Jump to bottom

Merged

twiecki merged 12 commits into master from tests

Feb 27, 2019

Contributor

springcoil commented Jan 11, 2019 •

edited

Loading

I couldn't get the eight schools model to work - PyMC4 didn't have half normal.

However I did get this to work, and the test passes. Do we want to add a tests file to the travis CI? Or is that too soon?

canyon289 reviewed

View reviewed changes

test-requirments.txt Outdated Show resolved Hide resolved

springcoil added the enhancement label

Contributor Author

springcoil commented Jan 11, 2019

@twiecki @fonnesbeck

Member

eigenfoo commented Jan 12, 2019

Not relevant to this PR, but once #41 gets merged we'll have all the distributions 😄

Contributor Author

springcoil commented Jan 12, 2019 via email

Ahh cool, then we'll be able to do some other tests

On Sat, Jan 12, 2019 at 6:05 PM George Ho ***@***.***> wrote: Not relevant to this PR, but once #41 <#41> gets merged we'll have all the distributions 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#39 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiCLznWy7jVYVcijf5fOq-DcjDiDxks5vCiPSgaJpZM4Z8BvC> .

-- Peadar Coyle Interested in levelling up your Machine Learning skills? Here is my Probabilistic Programming Primer <https://probabilisticprogrammingprimer.podia.com/>!!! Skype: springcoilarch www.twitter.com/springcoil peadarcoyle.wordpress.com

eigenfoo previously requested changes

View reviewed changes

Member

eigenfoo left a comment

Great work @springcoil! I know this looks like a boatload of comments, but most of them are just to do with readability and unnecessary code. I added suggestions where I could.

Just a few more comments though:

We can't call this file test_eight_schools! 😅 Maybe test_example_linear_regression?
Can you run black on this file, with the --py36 -l 100 flags? Alternatively you could rebase onto master.
Currently we test that the logp of the samples are equal to some pre-computed value. Shouldn't we also test that the samples themselves are equal to their pre-computed values?

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated

+              import tensorflow as tf
+              import numpy as np
+              import matplotlib.pyplot as plt
+              import pytest

Member

eigenfoo Jan 14, 2019

Unused import

Suggested change

import pytest

Member

eigenfoo Jan 16, 2019

We don't need to import pytest in our test files, do we? We don't do that in the PyMC3 test suite.

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated


		forward_sample = sess.run(model.forward_sample())

		forward_sample

Member

eigenfoo Jan 14, 2019

Unneeded

Suggested change

forward_sample

Member

eigenfoo Jan 16, 2019

Looks like it's still there :)

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

test-requirments.txt Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

pymc4/tests/test_eight_schools.py Outdated Show resolved Hide resolved

Contributor Author

springcoil commented Jan 14, 2019

I called this test_eight schools? I'm clearly not the brightest, I even left my coffee in a cafe this morning :)

Contributor Author

springcoil commented Jan 14, 2019

I'll fix these this evening Eigenfoo. Thanks for the constructive and thorough feedback :)

Contributor Author

springcoil commented Jan 14, 2019 via email

That was the plan - I could rename this though. ᐧ

On Mon, Jan 14, 2019 at 4:21 PM Ravin Kumar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test-requirments.txt <#39 (comment)>: > @@ -0,0 +1 @@ +pytest>=3.0.7 I would ask that we keep a requirements.txt and requirements-dev.txt or requirements-test.txt separate. This way we don't burden end users environment with things they don't need — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiC9piMjwJFGxGQ_H2-rZAOcX9kuZks5vDK5ugaJpZM4Z8BvC> .

-- Peadar Coyle Interested in levelling up your Machine Learning skills? Here is my Probabilistic Programming Primer <https://probabilisticprogrammingprimer.podia.com/>!!! Skype: springcoilarch www.twitter.com/springcoil peadarcoyle.wordpress.com

Member

ericmjl commented Jan 14, 2019

Currently we test that the logp of the samples are equal to some pre-computed value.

I learned something today, @eigenfoo! This is a great idea for testing.

canyon289 mentioned this pull request

Local Pytest and docker TravisCI testing #49

Merged

springcoil force-pushed the tests branch 2 times, most recently from e96bbb9 to d1d9650 Compare

January 15, 2019 10:37

springcoil dismissed eigenfoo’s stale review

January 15, 2019 10:37

I've done the changes as he asked :)

Member

eigenfoo commented Jan 16, 2019

Travis tests failed. You just need to format it with black! black -l 100 --py36 tests/ should do the trick.

Contributor Author

springcoil commented Jan 16, 2019 via email

I don't think so. Maybe remove it.

On Wed, 16 Jan 2019, 3:48 pm George Ho ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc4/tests/test_eight_schools.py <#39 (comment)>: > @@ -0,0 +1,49 @@ +import pymc4 as pm +import pymc3 as pm3 +import tensorflow as tf +import numpy as np +import matplotlib.pyplot as plt +import pytest We don't need to import pytest in our test files, do we? We don't do that in the PyMC3 test suite. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiNnUlWhJFGk8asL_e43VreLRvpIQks5vD0nSgaJpZM4Z8BvC> .

springcoil force-pushed the tests branch 2 times, most recently from 0eb4aa0 to 7fc5dba Compare

January 16, 2019 23:52

Contributor Author

springcoil commented Jan 17, 2019

@eigenfoo I took your advice and added in a sample test :)

twiecki reviewed

View reviewed changes

pymc4/tests/test_linearregression_model.py Outdated


		model = linreg.configure()

		forward_sample = sess.run(model.forward_sample())

Member

twiecki Jan 17, 2019

Once #49 is merged should use that fixture.

Contributor Author

springcoil Jan 17, 2019

Agree wholeheartedly.

Contributor Author

springcoil Jan 17, 2019

Is this ready to merge?

Member

twiecki commented Jan 19, 2019

we should port this to the new testing framework m

Contributor Author

springcoil commented Jan 19, 2019 via email

Yeah that's fine, I'll have a look on Monday.

On Sat, 19 Jan 2019, 1:44 pm Thomas Wiecki ***@***.*** wrote: we should port this to the new testing framework m — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiD2uazmVty1JWMVt_YSjjrDymGJxks5vExMzgaJpZM4Z8BvC> .

Contributor Author

springcoil commented Jan 25, 2019

@canyon289 I might not have a lot of time over the next few weeks. So if you wanted to just copy and paste the test into your stuff - you can :)

Member

canyon289 commented Feb 16, 2019

I can work on deconflicting this branch to prep for a merge in the next ~36 hours. Any naysayers for proposal?

Contributor Author

springcoil commented Feb 16, 2019 via email

Go on ahead

On Sat, 16 Feb 2019, 5:07 pm Ravin Kumar ***@***.*** wrote: I can work on deconflicting this branch to prep for a merge in the next ~36 hours. Any naysayers for proposal? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiHd6G_pMNUF0ACETrv8cuc_VOaiTks5vOCzdgaJpZM4Z8BvC> .

springcoil added 4 commits

February 18, 2019 08:13


          Tests: Simple sense test for linear regression

8e1c284

Adding renaming

Adding linear regression again

Fixing black errors


          Added a forward sample test

b8a9c0e


          Black formatting

b279d4b


          Made this a fixture test

59fec5e

canyon289 force-pushed the tests branch from 71f1026 to 59fec5e Compare

February 18, 2019 16:13

canyon289 added 2 commits

February 18, 2019 08:14


          Delete random variables again

8bdbe15


          Add tf_session fixture to clear tensorflow graph

ac0cb79

Member

canyon289 commented Feb 18, 2019

@springcoil Can you review and let me know if this still meets your intent?

springcoil commented

View reviewed changes

Contributor Author

springcoil left a comment

Lgtm

Member

canyon289 commented Feb 18, 2019

Seems like there's just enough floating point error to cause the tests to fail. I'll make adjustments in the next couple of days.

canyon289 added 4 commits

February 23, 2019 12:40


          Increase decimal tolerance of test_linearregression

26a2c9a


          Remove exact requirement from test

72c048b


          Relax decimal requirement on another equal assert

92aa746


          Remove errant decimal

e4ee5d8

Member

canyon289 commented Feb 26, 2019

This is ready for review again

twiecki reviewed

View reviewed changes

pymc4/tests/test_linearregression_model.py Outdated

		@@ -0,0 +1,172 @@
		import pymc4 as pm

Member

twiecki Feb 26, 2019

should probably be called test_linear_regression_model.py

twiecki reviewed

View reviewed changes

pymc4/tests/test_linearregression_model.py Outdated

		forward_sample = sess.run(model.forward_sample())


		func = model.make_log_prob_function()

Member

twiecki Feb 26, 2019

should these all hang out in the module namespace?

Contributor Author

springcoil commented Feb 26, 2019 via email

Agreed they should probably not be globals. ᐧ

On Tue, Feb 26, 2019 at 10:29 AM Thomas Wiecki ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc4/tests/test_linearregression_model.py <#39 (comment)>: > + # Define priors + sigma = pm.HalfNormal("sigma", scale=10) + intercept = pm.Normal("intercept", 0, scale=10) + x_coeff = pm.Normal("weight", 0, scale=5) + x = np.linspace(-5, 5, n_points) + + # Define likelihood + y = pm.Normal("y", loc=intercept + x_coeff * x, scale=sigma) + + +model = linreg.configure() + +forward_sample = sess.run(model.forward_sample()) + + +func = model.make_log_prob_function() should these all hang out in the module namespace? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiPQa6e_-fNhauHyNOkDk30iuofZnks5vRQyAgaJpZM4Z8BvC> .

-- Peadar Coyle Interested in levelling up your Machine Learning skills? Here is my Probabilistic Programming Primer <https://probabilisticprogrammingprimer.podia.com/>!!! Skype: springcoilarch www.twitter.com/springcoil peadarcoyle.wordpress.com

canyon289 added 2 commits

February 26, 2019 07:51


          Refactor linear regression test

40a3c4b


          Rename test_linear_regression

4dd7818

twiecki merged commit 8c4d6ad into master

twiecki deleted the tests branch

February 27, 2019 13:42

Member

twiecki commented Feb 27, 2019

Thanks @canyon289!

Member

canyon289 commented Feb 27, 2019

And @springcoil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels