Pytest with 89% coverage by rflamary · Pull Request #19

Pytest with 89% coverage by rflamary · Pull Request #19 · PythonOT/POT

Add numerous test for existing functions and classes.
Correct failing build due to Python3/2 map function difference.

Will merge soon since currently POT do not build.

rflamary changed the title ~~Pytest with 85% coverage~~ Pytest with 89% coverage

Jul 24, 2017

POT now build with a certain number of tests. I will merge this PR today unless somebody objects.

		before_script: # configure a headless display to test plot generation
		- "export DISPLAY=:99.0"
		- "sh -e /etc/init.d/xvfb start"
		- sleep 3 # give xvfb some time to start

Choose a reason for hiding this comment

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

do you use only matplotlib? if so just use the Agg backend

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

that's weird. You seem to have done it right. Are you sure matplotlib is not imported anywhere before?

you should also nest the imports to matplotlib in the or source tree. So matplotlib is not imported when you do import ot


		pytest : FORCE
		python -m py.test -v test/
		python -m py.test -v test/ --cov=ot

Choose a reason for hiding this comment

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

you should have a native pytest command:

pytest -v test/ --cov=ot

Choose a reason for hiding this comment

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

Under Debian/Ubuntu logilab-common install a useless executable named pytest. It's a well known bug but takes time to be corrected. This line ensure that the proper py.test is executed.

pytest-dev/pytest#1833

		reg = 1e-3
		bary_wass = ot.bregman.barycenter(A, M, reg, weights)

		assert np.allclose(1, np.sum(bary_wass))

Choose a reason for hiding this comment

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

you have an assert_allclose function in numpy

@rflamary please wait. I'll do a proper review in the next 2 days.

		def test_sinkhorn_empty():
		# test sinkhorn
		n = 100
		np.random.seed(0)

Choose a reason for hiding this comment

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

use a random state

rng = np.random.RandomState(42)
x = rng.randn(n, 2)

etc.

ie don't change the global seed.

		G, log = ot.sinkhorn([], [], M, 1, stopThr=1e-10, verbose=True, log=True)
		# check constratints
		assert np.allclose(u, G.sum(1), atol=1e-05) # cf convergence sinkhorn
		assert np.allclose(u, G.sum(0), atol=1e-05) # cf convergence sinkhorn

Choose a reason for hiding this comment

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

use np.testing.assert_allclose

it makes errors clearer than just an assert


		# Gaussian distributions
		a1 = ot.datasets.get_1D_gauss(n, m=30, s=10) # m= mean, s= std
		a2 = ot.datasets.get_1D_gauss(n, m=40, s=10)

Choose a reason for hiding this comment

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

as I was saying it should be named in the future

make_1d_gauss


		def test_unmix():

		n = 50 # nb bins

Choose a reason for hiding this comment

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

n -> n_bins

as n can mean n_samples etc.

if you call it n_bins no need to write nb bins :)



		import ot
		import numpy as np

Choose a reason for hiding this comment

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

import numpy before ot
as ot depends on numpy

it's for convention

		# import pytest


		def test_OTDA():

Choose a reason for hiding this comment

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

test_OTDA -> test_otda

no caps in function names

		n = 150 # nb bins

		xs, ys = ot.datasets.get_data_classif('3gauss', n)
		xt, yt = ot.datasets.get_data_classif('3gauss2', n)

Choose a reason for hiding this comment

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

get_data_classif -> make_classification

would be sklearn consistent.

Choose a reason for hiding this comment

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

OK we should open an issue and handle that in a separate PR I think, this one is mainly for testing

		import pytest

		try: # test if cudamat installed
		import ot.dr

Choose a reason for hiding this comment

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

test for what you really need to test ie if cudamat is available

try:
    import cudamat
    has_cudamat = False
except ...:
    has_cudamat = True

OK @agramfort I took care of most your reviews.

What remains and will be opened as Issues :

Renaming dataset function to be more sklearn compliant (breaking change)
Weird travis fail with no open DISPLAY

If the travis build work I will merge the PR since I introduced no features in the toolbox only tests.

		def test_otda():

		n_samples = 150 # nb samples
		np.random.seed(0)

Choose a reason for hiding this comment

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

RandomState

the get_data_classif function should take the rng in param and use it instead of np.random.randn

see the check_random_state function in sklearn

		def test_conditional_gradient():

		n_bins = 100 # nb bins
		np.random.seed(0)

Choose a reason for hiding this comment

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

RandomState

		l2 = ot.utils.parmap(f, a)
		l2 = list(ot.utils.parmap(f, a))

		assert np.allclose(l1, l2)

Choose a reason for hiding this comment

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

use np.testing.assert_allclose


		# dist shoul return squared euclidean
		assert np.allclose(D, D2)
		assert np.allclose(D, D3)

Choose a reason for hiding this comment

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

idem

		M = ot.utils.dist0(n, method='lin_square')

		# dist0 default to linear sampling with quadratic loss
		assert np.allclose(M[0, -1], (n - 1) * (n - 1))

Choose a reason for hiding this comment

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

idem and below too

ok no more nitpicks after these

OK great thank you again,

I won't handle the rng stuff in this PR I will add it to the Issue about the make_datasets function.

OK let's merge this PR,

We now have a 89% coverage of the code when all libraries are installed (cudamat, pymanopt, autograd).

Also the Makefile include the test target that checks for PEP8 violations and and run the tests.

I have created Issue #20 for the dataset function names and random state problems.