pymc
pymc copied to clipboard
Remove `samples` and `keep_size` from `sample_posterior_predictive`
Removed samples
and keep_size
from sample_posterior_predictive
in sampling.py
In response to issue #5775, we removed the samples
and keep_size
parameters on the sample_posterior_predictive
function. Consequently, we removed all the calls to these two variables inside the functions and edited the code such that it runs as keep_size
was always True
even though the variable is not actually defined.
Regarding the samples
parameter, it is defined by the trace object inside the function, so there was no need for it be a function parameter.
Moreover, we edited the all test functions that used these two parameters.
Pre-commit and linting tests were passed.
#DataUmbrellaPyMCSprint P.D.: Thanks for all the help to @OriolAbril ! cc: @vitaliset @OriolAbril @reshamas
Codecov Report
Merging #6029 (37f3ebf) into main (eaa51f3) will decrease coverage by
0.00%
. The diff coverage is93.33%
.
Additional details and impacted files
@@ Coverage Diff @@
## main #6029 +/- ##
==========================================
- Coverage 93.58% 93.58% -0.01%
==========================================
Files 101 101
Lines 22151 22136 -15
==========================================
- Hits 20731 20716 -15
Misses 1420 1420
Impacted Files | Coverage Δ | |
---|---|---|
pymc/tests/backends/test_arviz.py | 99.00% <ø> (-0.03%) |
:arrow_down: |
pymc/sampling.py | 82.48% <87.50%> (-0.16%) |
:arrow_down: |
pymc/tests/distributions/test_mixture.py | 99.35% <100.00%> (ø) |
|
pymc/tests/distributions/test_simulator.py | 99.50% <100.00%> (ø) |
|
pymc/model.py | 88.26% <0.00%> (+0.03%) |
:arrow_up: |
Thanks!
I took a quick look at the test failures. They are due to the lack of chain dimension in the asserts in many/most cases:
- https://github.com/pymc-devs/pymc/runs/7692438573?check_suite_focus=true#step:7:1501 for test
TestMixture.test_single_poisson_predictive_sampling_shape
- https://github.com/pymc-devs/pymc/runs/7692438573?check_suite_focus=true#step:7:1577 for test
TestMixture.test_list_mvnormals_predictive_sampling_shape
- https://github.com/pymc-devs/pymc/runs/7692439777?check_suite_focus=true#step:7:250 for test
TestSamplePPC.test_normal_scalar
Can you take a look at the logs and see if you can update the asserts on shapes that need to be modified?
Thanks for looking at the logs, @OriolAbril. I will take care of that during the weekend!
@OriolAbril @pibieta @vitaliset Do we know why the tests are failing here?
Yes, we have an idea @reshamas . I will work on it today, I just didn't have time to work on this PR.
@pibieta @vitaliset Let me know if you want me to rehash what we discussed on Friday. Thanks again for your contribution(s)!
Hi @pibieta @vitaliset, Let us know if you have any questions on this PR.
I think there was a question (probably for @ricardoV94 ) about the p-value in one failing test. That was at least 1 remaining thing. There may have been others.
Which test is it? Is the test failing systematically when running locally?
Thanks for your work @pibieta and @vitaliset, I hope you don't mind I took over the PR to get it over the finish line. I will do further changes to sample_posterior_predictive (see #6208 ) and having merged this will make things a bit easier. Moreover, my other PR would have introduced many more git conflicts if merged before this one, making it even more hard to get this PR back to good shape and ready to merge.
Thanks for taking over this PR, @OriolAbril ! We were actually having trouble to fix the tests, so I think the best was for you to take over, haha. We will be working on other issues from now on. Thanks again, Oriol!