Skip to content

Unseed (and slightly refactor) TestMatchesScipy #4461

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

Merged
merged 5 commits into from
Feb 8, 2021

Conversation

ricardoV94
Copy link
Member

This PR unseeds TestMatchesScipy, which contradicted the effect of limiting the number of samples in product(domains, n_samples) which is supposed to return a random subset of domain x paramdomain points that are used to test the logp and logcdf methods.

This, for instance, made this last PR #4459 somewhat shortsighted in that it assumed we would keep covering the entire domain grid (over multiple CI runs)

I also refactored the old pymc3_matches_scipy to check_logp, in line with the check_logcdf that was added at a later point. I added some documentation, mostly borrowing from the check_locgdf function.


Depending on what your PR does, here are a few things you might want to address in the description:

@ricardoV94 ricardoV94 requested a review from ColCarroll February 5, 2021 09:55
@ricardoV94 ricardoV94 changed the title Refactor and unseed TestMatchesScipy Unseed (and slightly refactor) TestMatchesScipy Feb 5, 2021
@ricardoV94
Copy link
Member Author

ricardoV94 commented Feb 5, 2021

Seems like we have a numerical precision issue in the skewnormal: https://github.com/pymc-devs/pymc3/pull/4461/checks?check_run_id=1837831640

Reducing the 64bit decimal precision from 6 to 5, seems to work (I tested on all combinations locally)

@ricardoV94
Copy link
Member Author

ricardoV94 commented Feb 5, 2021

Precision issues in gamma logcdf (32-bit): {'alpha': array(20., dtype=float32), 'beta': array(0.9, dtype=float32), 'value': array(0.01, dtype=float32)} -> -inf in the PyMC3 implementation

@ricardoV94 ricardoV94 force-pushed the unseed_TestMatchesScipy branch from 5fe7bc6 to d0d0e0a Compare February 5, 2021 13:30
@ricardoV94 ricardoV94 force-pushed the unseed_TestMatchesScipy branch from d0d0e0a to 4f98dba Compare February 5, 2021 13:37
@ricardoV94
Copy link
Member Author

InverseGamma logp also failed with 6 decimals, but passes with 5 (all combinations tested locally).

I am now testing all methods locally with all the samples (64bit for now). I figured out that it's probably better to do a complete run once rather than keep chasing the failures...

@ricardoV94
Copy link
Member Author

My only lingering concern is whether check_int_to_1 has any randomness to it, which could benefit from seeding. Does anyone know? If yes, I would move the 7 or so tests that use it a separate Seeded class.

https://github.com/pymc-devs/pymc3/blob/60e0daea62f1d3a024bc62aca6703ce0eafbdea5/pymc3/tests/test_distributions.py#L735

@michaelosthege
Copy link
Member

My only lingering concern is whether check_int_to_1 has any randomness to it, which could benefit from seeding. Does anyone know? If yes, I would move the 7 or so tests that use it a separate Seeded class.

https://github.com/pymc-devs/pymc3/blob/60e0daea62f1d3a024bc62aca6703ce0eafbdea5/pymc3/tests/test_distributions.py#L735

integrate_nd uses scipy.integrate.quad which should be deterministic as far as I know.

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

LGTM

@ricardoV94 ricardoV94 merged commit e40304b into pymc-devs:master Feb 8, 2021
@ricardoV94 ricardoV94 deleted the unseed_TestMatchesScipy branch April 22, 2021 07:26
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.

2 participants