Skip to content

Break down tests having multiple checks and xfail decorated #4497

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

Merged
merged 15 commits into from
Mar 9, 2021
Merged

Break down tests having multiple checks and xfail decorated #4497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8700a6b
Break down tests having multiple checks and xfail decorated
matteo-pallini Mar 2, 2021
99c6e49
Remove xfail which seems unnecessary
matteo-pallini Mar 2, 2021
80d2711
Break down test with multiple sub-checks and xfail where appropriate
matteo-pallini Mar 2, 2021
56d619f
Remove Scipy Xfail from tests not using sp.betabinom
matteo-pallini Mar 3, 2021
060d164
Add back xfail to potentially failing test when float32 is used
matteo-pallini Mar 3, 2021
c15751b
Temporatily add n_samples=-1 to check tests behavior for all possible…
matteo-pallini Mar 3, 2021
09b9ff0
Remove temporary n_samples and a couple more to do some quick tests
matteo-pallini Mar 3, 2021
e54934a
Remove unnecessary comments
matteo-pallini Mar 3, 2021
474d0b8
Replace Xfail with skipif when using Scipy version as condition
matteo-pallini Mar 3, 2021
3181591
Add missing n_samples for test_beta_binomial_selfconsistency
matteo-pallini Mar 3, 2021
7407991
Remove all the n_samples=-1 added for verifying tests robustness
matteo-pallini Mar 3, 2021
94e700d
Tweak normal_logcdf and moyal tests
ricardoV94 Mar 4, 2021
48c061f
Reintroduce 32bit xfail on Moyal logcdf, revert n_samples and merge p…
ricardoV94 Mar 4, 2021
99b119a
Small renaming and reordering
ricardoV94 Mar 4, 2021
32de1a4
Reintroduce accidentally removed n_samples from binomial test
ricardoV94 Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 82 additions & 41 deletions pymc3/tests/test_distributions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,9 +770,9 @@ def check_selfconsistency_discrete_logcdf(
err_msg=str(pt),
)

def check_int_to_1(self, model, value, domain, paramdomains):
def check_int_to_1(self, model, value, domain, paramdomains, n_samples=10):
pdf = model.fastfn(exp(model.logpt))
for pt in product(paramdomains, n_samples=10):
for pt in product(paramdomains, n_samples=n_samples):
pt = Point(pt, value=value.tag.test_value, model=model)
bij = DictToVarBijection(value, (), pt)
pdfx = bij.mapf(pdf)
Expand Down Expand Up @@ -902,6 +902,7 @@ def test_normal(self):
R,
{"mu": R, "sigma": Rplus},
lambda value, mu, sigma: sp.norm.logcdf(value, mu, sigma),
decimal=select_by_precision(float64=6, float32=2),
Copy link
Member

Choose a reason for hiding this comment

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

float32 precision of 2 digits sounds awfully bad. Are you sure about this?

Copy link
Member

@ricardoV94 ricardoV94 Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's actually better than the normal::logp (this one is the normal::logcdf) test which uses a precision of 1! (Previous to this PR)

Copy link
Member

Choose a reason for hiding this comment

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

This used to pass before because these tests were all seeded until very recently and the conditions with low precision were not appearing / failing since then

)

def test_truncated_normal(self):
Expand Down Expand Up @@ -941,25 +942,6 @@ def test_chi_squared(self):
lambda value, nu: sp.chi2.logpdf(value, df=nu),
)

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Poor CDF in SciPy. See scipy/scipy#869 for details.",
)
def test_wald_scipy(self):
self.check_logp(
Wald,
Rplus,
{"mu": Rplus, "alpha": Rplus},
lambda value, mu, alpha: sp.invgauss.logpdf(value, mu=mu, loc=alpha),
decimal=select_by_precision(float64=6, float32=1),
)
self.check_logcdf(
Wald,
Rplus,
{"mu": Rplus, "alpha": Rplus},
lambda value, mu, alpha: sp.invgauss.logcdf(value, mu=mu, loc=alpha),
)

@pytest.mark.parametrize(
"value,mu,lam,phi,alpha,logp",
[
Expand All @@ -979,7 +961,7 @@ def test_wald_scipy(self):
(50.0, 15.0, None, 0.666666, 10.0, -5.6481874),
],
)
def test_wald(self, value, mu, lam, phi, alpha, logp):
def test_wald_logp_custom_points(self, value, mu, lam, phi, alpha, logp):
# Log probabilities calculated using the dIG function from the R package gamlss.
# See e.g., doi: 10.1111/j.1467-9876.2005.00510.x, or
# http://www.gamlss.org/.
Expand All @@ -989,6 +971,27 @@ def test_wald(self, value, mu, lam, phi, alpha, logp):
decimals = select_by_precision(float64=6, float32=1)
assert_almost_equal(model.fastlogp(pt), logp, decimal=decimals, err_msg=str(pt))

def test_wald_logp(self):
self.check_logp(
Wald,
Rplus,
{"mu": Rplus, "alpha": Rplus},
lambda value, mu, alpha: sp.invgauss.logpdf(value, mu=mu, loc=alpha),
decimal=select_by_precision(float64=6, float32=1),
Copy link
Member

Choose a reason for hiding this comment

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

Here too - 1 digit precision?!

Copy link
Member

@ricardoV94 ricardoV94 Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This preceeded this PR

Copy link
Member

@ricardoV94 ricardoV94 Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

By the way these 1 precision comparisons are not necessarily as bad as they may sound. It's usually when the log becomes very negative, close to underflowing to -inf. things like -213.161 vs -213.172 that mean basically nothing.

Also many times we are dealing with loss of precision on the scipy side, not on our side.

)

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Poor CDF in SciPy. See scipy/scipy#869 for details.",
)
def test_wald_logcdf(self):
self.check_logcdf(
Wald,
Rplus,
{"mu": Rplus, "alpha": Rplus},
lambda value, mu, alpha: sp.invgauss.logcdf(value, mu=mu, loc=alpha),
)

def test_beta(self):
self.check_logp(
Beta,
Expand Down Expand Up @@ -1256,11 +1259,7 @@ def test_gamma_logcdf(self):
skip_paramdomain_outside_edge_test=True,
)

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to numerical issues",
)
def test_inverse_gamma(self):
def test_inverse_gamma_logp(self):
self.check_logp(
InverseGamma,
Rplus,
Expand All @@ -1269,6 +1268,14 @@ def test_inverse_gamma(self):
)
# pymc-devs/aesara#224: skip_paramdomain_outside_edge_test has to be set
# True to avoid triggering a C-level assertion in the Aesara GammaQ function

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to numerical issues",
)
def test_inverse_gamma_logcdf(self):
# pymc-devs/aesara#224: skip_paramdomain_outside_edge_test has to be set
# True to avoid triggering a C-level assertion in the Aesara GammaQ function
# in gamma.c file. Can be set back to False (default) once that issue is solved
self.check_logcdf(
InverseGamma,
Expand Down Expand Up @@ -1313,13 +1320,19 @@ def test_pareto(self):
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to inf issues",
)
def test_weibull(self):
def test_weibull_logp(self):
self.check_logp(
Weibull,
Rplus,
{"alpha": Rplusbig, "beta": Rplusbig},
lambda value, alpha, beta: sp.exponweib.logpdf(value, 1, alpha, scale=beta),
)

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to inf issues",
)
def test_weibull_logcdf(self):
self.check_logcdf(
Weibull,
Rplus,
Expand Down Expand Up @@ -1368,27 +1381,37 @@ def test_binomial(self):

# Too lazy to propagate decimal parameter through the whole chain of deps
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
@pytest.mark.xfail(
condition=(SCIPY_VERSION < parse("1.4.0")), reason="betabinom is new in Scipy 1.4.0"
)
def test_beta_binomial(self):
def test_beta_binomial_distribution(self):
self.checkd(
BetaBinomial,
Nat,
{"alpha": Rplus, "beta": Rplus, "n": NatSmall},
)

@pytest.mark.skipif(
condition=(SCIPY_VERSION < parse("1.4.0")), reason="betabinom is new in Scipy 1.4.0"
)
def test_beta_binomial_logp(self):
self.check_logp(
BetaBinomial,
Nat,
{"alpha": Rplus, "beta": Rplus, "n": NatSmall},
lambda value, alpha, beta, n: sp.betabinom.logpmf(value, a=alpha, b=beta, n=n),
)

@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
@pytest.mark.skipif(
condition=(SCIPY_VERSION < parse("1.4.0")), reason="betabinom is new in Scipy 1.4.0"
)
def test_beta_binomial_logcdf(self):
self.check_logcdf(
BetaBinomial,
Nat,
{"alpha": Rplus, "beta": Rplus, "n": NatSmall},
lambda value, alpha, beta, n: sp.betabinom.logcdf(value, a=alpha, b=beta, n=n),
)

def test_beta_binomial_selfconsistency(self):
self.check_selfconsistency_discrete_logcdf(
BetaBinomial,
Nat,
Expand Down Expand Up @@ -1475,27 +1498,37 @@ def test_constantdist(self):
self.check_logp(Constant, I, {"c": I}, lambda value, c: np.log(c == value))

# Too lazy to propagate decimal parameter through the whole chain of deps
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_zeroinflatedpoisson(self):
@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to inf issues",
)
def test_zeroinflatedpoisson_distribution(self):
self.checkd(
ZeroInflatedPoisson,
Nat,
{"theta": Rplus, "psi": Unit},
)

def test_zeroinflatedpoisson_logcdf(self):
self.check_selfconsistency_discrete_logcdf(
ZeroInflatedPoisson,
Nat,
{"theta": Rplus, "psi": Unit},
)

# Too lazy to propagate decimal parameter through the whole chain of deps
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_zeroinflatednegativebinomial(self):
@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Fails on float32 due to inf issues",
)
def test_zeroinflatednegativebinomial_distribution(self):
self.checkd(
ZeroInflatedNegativeBinomial,
Nat,
{"mu": Rplusbig, "alpha": Rplusbig, "psi": Unit},
)

def test_zeroinflatednegativebinomial_logcdf(self):
self.check_selfconsistency_discrete_logcdf(
ZeroInflatedNegativeBinomial,
Nat,
Expand All @@ -1504,13 +1537,14 @@ def test_zeroinflatednegativebinomial(self):
)

# Too lazy to propagate decimal parameter through the whole chain of deps
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_zeroinflatedbinomial(self):
def test_zeroinflatedbinomial_distribution(self):
self.checkd(
ZeroInflatedBinomial,
Nat,
{"n": NatSmall, "p": Unit, "psi": Unit},
)

def test_zeroinflatedbinomial_logcdf(self):
self.check_selfconsistency_discrete_logcdf(
ZeroInflatedBinomial,
Nat,
Expand Down Expand Up @@ -2279,14 +2313,21 @@ def test_rice(self):
lambda value, b, sigma: sp.rice.logpdf(value, b=b, loc=0, scale=sigma),
)

@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_moyal(self):
def test_moyal_logp(self):
# Using a custom domain, because the standard `R` domain undeflows with scipy in float64
value_domain = Domain([-inf, -1.5, -1, -0.01, 0.0, 0.01, 1, 1.5, inf])
self.check_logp(
Moyal,
R,
value_domain,
{"mu": R, "sigma": Rplusbig},
lambda value, mu, sigma: floatX(sp.moyal.logpdf(value, mu, sigma)),
)

@pytest.mark.xfail(
condition=(aesara.config.floatX == "float32"),
reason="Pymc3 underflows earlier than scipy on float32",
)
def test_moyal_logcdf(self):
self.check_logcdf(
Moyal,
R,
Expand Down
3 changes: 2 additions & 1 deletion pymc3/tests/test_examples.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ def build_disaster_model(masked=False):
return model


@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
class TestDisasterModel(SeededTest):
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
# Time series of recorded coal mining disasters in the UK from 1851 to 1962
def test_disaster_model(self):
model = build_disaster_model(masked=False)
Expand All @@ -205,6 +205,7 @@ def test_disaster_model(self):
tr = pm.sample(500, tune=50, start=start, step=step, chains=2)
az.summary(tr)

@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_disaster_model_missing(self):
model = build_disaster_model(masked=True)
with model:
Expand Down
3 changes: 2 additions & 1 deletion pymc3/tests/test_models_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def test_pandas_init(self):
m, l = utils.any_to_tensor_and_labels(self.data, labels=["x2", "x3"])
self.assertMatrixLabels(m, l, lt=["x2", "x3"])

@pytest.mark.xfail
def test_dict_input(self):
m, l = utils.any_to_tensor_and_labels(self.data.to_dict("dict"))
self.assertMatrixLabels(m, l, mt=self.data[l].values, lt=l)
Expand All @@ -51,6 +50,8 @@ def test_dict_input(self):
m, l = utils.any_to_tensor_and_labels(self.data.to_dict("list"))
self.assertMatrixLabels(m, l, mt=self.data[l].values, lt=l)

@pytest.mark.xfail
Copy link
Member

Choose a reason for hiding this comment

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

Can we dig the reason for this one?

Copy link
Contributor Author

@matteo-pallini matteo-pallini Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that pymc3.glm.utils.any_to_tensor_and_labels doesn't support converting data structures containing TensorType (mostly because theano.tensor.as_tensor_variable called on line 132 doesn't). So, in this case inp will cause an error to be raised

I haven't been able to understand what conditions made this test pass. I guess that if we need pymc3.glm.utils.any_to_tensor_and_labels to be able to handle data structure containing tensor objects we should probably change that

Copy link
Member

Choose a reason for hiding this comment

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

I tracked it down to this commit: 4f67d63

@rpgoldman could you chime in? What is the source of this failure? It would be helpful to have a more clear reason specified in the xfail.

Copy link
Member

Choose a reason for hiding this comment

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

The xfail was there before, so this shouldn't block us from moving forward with this PR.

def test_dict_input_pandas_series(self):
inp = {k: aet.as_tensor_variable(v.values) for k, v in self.data.to_dict("series").items()}
m, l = utils.any_to_tensor_and_labels(inp)
self.assertMatrixLabels(m, l, mt=self.data[l].values, lt=l)
Expand Down
2 changes: 1 addition & 1 deletion pymc3/tests/test_step.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,8 +957,8 @@ def test_custom_proposal_dist(self):
pass


@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
class TestNutsCheckTrace:
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_multiple_samplers(self, caplog):
with Model():
prob = Beta("prob", alpha=5.0, beta=3.0)
Expand Down
15 changes: 11 additions & 4 deletions pymc3/tests/test_transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,22 @@ def test_ordered():
close_to_logical(np.diff(vals) >= 0, True, tol)


def test_chain_values():
chain_tranf = tr.Chain([tr.logodds, tr.ordered])
vals = get_values(chain_tranf, Vector(R, 5), aet.dvector, np.zeros(5))
close_to_logical(np.diff(vals) >= 0, True, tol)


@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_chain():
def test_chain_vector_transform():
chain_tranf = tr.Chain([tr.logodds, tr.ordered])
check_vector_transform(chain_tranf, UnitSortedVector(3))

check_jacobian_det(chain_tranf, Vector(R, 4), aet.dvector, np.zeros(4), elemwise=False)

vals = get_values(chain_tranf, Vector(R, 5), aet.dvector, np.zeros(5))
close_to_logical(np.diff(vals) >= 0, True, tol)
@pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
def test_chain_jacob_det():
chain_tranf = tr.Chain([tr.logodds, tr.ordered])
check_jacobian_det(chain_tranf, Vector(R, 4), aet.dvector, np.zeros(4), elemwise=False)


class TestElementWiseLogp(SeededTest):
Expand Down