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

Seed flaky test TestSamplePPC.test_normal_scalar #6220

Merged
merged 5 commits into from
Oct 21, 2022

Conversation

mattiadg
Copy link
Contributor

@mattiadg mattiadg commented Oct 16, 2022

This is related to the issue

  • Remove call to function with unused output
  • Add assert to test function with list input
  • Add comment to refer to the issue

What is this PR about?
The issue Flaky test: test_normal_scalar #6211 reported a Flaky test in test_normal_scalar https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_sampling.py#L617. I could not reproduce it yet, but I found other minor issues in the same test: calls to the tested function that were not calling any assert. I added an assert after the line with an input different from the other calls, and removed the other line that looks redundant. Under the suggestion of @michaelosthege, I added a comment asking to report further failing of this test under the same issue

Checklist

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • Seed flaky test TestSamplePPC.test_normal_scalar

This is related to the issue Flaky test: test_normal_scalar pymc-devs#6211 pymc-devs#6211

* Remove call to function with unused output
* Add assert to test function with list input
* Add comment to refer to the issue
@codecov
Copy link

codecov bot commented Oct 16, 2022

Codecov Report

Merging #6220 (bfc7452) into main (d47dac0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6220      +/-   ##
==========================================
+ Coverage   93.58%   93.61%   +0.02%     
==========================================
  Files         101      101              
  Lines       22136    22120      -16     
==========================================
- Hits        20716    20707       -9     
+ Misses       1420     1413       -7     
Impacted Files Coverage Δ
pymc/backends/ndarray.py 79.27% <ø> (-0.19%) ⬇️
pymc/backends/base.py 87.95% <100.00%> (+0.84%) ⬆️
pymc/sampling.py 82.62% <100.00%> (+0.14%) ⬆️
pymc/tests/backends/fixtures.py 78.30% <100.00%> (+0.31%) ⬆️

@mattiadg
Copy link
Contributor Author

Apparently, that call was covering some additional lines but no assert was there. I will understand what it was supposed to test and add an assert accordingly

@michaelosthege
Copy link
Member

Apparently, that call was covering some additional lines but no assert was there. I will understand what it was supposed to test and add an assert accordingly

Not sure what you mean. For me this looks all good already.

@michaelosthege
Copy link
Member

Relevant comment: #6210 (comment)

Looks like the SeededTest does not affect posterior predictive sampling?!

@mattiadg
Copy link
Contributor Author

Found the problem, we need to pass explicitly the random_seed, actually an object of type RandomState to pm.sample_posterior_predictive. This way I get the same samples. New commit follows in a while

This really makes the samples fixed and reproducible. There are functions called by sample_posterior_predictive that ignore the global random seed if one is not provided explicitely.
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks great, just a request to minimize the PR changes

Comment on lines +649 to +651
idata_ppc = pm.sample_posterior_predictive(
trace, var_names=["a"], random_seed=random_state
)
Copy link
Member

@ricardoV94 ricardoV94 Oct 19, 2022

Choose a reason for hiding this comment

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

Only this test requires a seed, the others are not checking anything that depends on the draws. Although passing a seed doesn't hurt, it makes reading the git history less straightforward (unless it was a commit about adding random_seed to all tests, and not just this one in particular).

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 see, removed the seed from all the others.

@ricardoV94 ricardoV94 changed the title Partially fix test_normal_scalar Seed flaky test TestSamplePPC.test_normal_scalar Oct 20, 2022
@ricardoV94 ricardoV94 merged commit 49ad534 into pymc-devs:main Oct 21, 2022
@ricardoV94
Copy link
Member

Thanks @mattiadg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants