pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Remove `samples` and `keep_size` from `sample_posterior_predictive`

Open pibieta opened this issue 2 years ago • 9 comments

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

pibieta avatar Aug 05 '22 13:08 pibieta

Codecov Report

Merging #6029 (37f3ebf) into main (eaa51f3) will decrease coverage by 0.00%. The diff coverage is 93.33%.

Additional details and impacted files

Impacted file tree graph

@@            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:

codecov[bot] avatar Aug 05 '22 14:08 codecov[bot]

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?

OriolAbril avatar Aug 05 '22 19:08 OriolAbril

Thanks for looking at the logs, @OriolAbril. I will take care of that during the weekend!

pibieta avatar Aug 05 '22 19:08 pibieta

@OriolAbril @pibieta @vitaliset Do we know why the tests are failing here?

reshamas avatar Aug 17 '22 12:08 reshamas

Yes, we have an idea @reshamas . I will work on it today, I just didn't have time to work on this PR.

pibieta avatar Aug 17 '22 13:08 pibieta

@pibieta @vitaliset Let me know if you want me to rehash what we discussed on Friday. Thanks again for your contribution(s)!

cluhmann avatar Aug 20 '22 19:08 cluhmann

Hi @pibieta @vitaliset, Let us know if you have any questions on this PR.

reshamas avatar Sep 02 '22 16:09 reshamas

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.

cluhmann avatar Sep 02 '22 16:09 cluhmann

Which test is it? Is the test failing systematically when running locally?

ricardoV94 avatar Sep 02 '22 16:09 ricardoV94

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.

OriolAbril avatar Oct 11 '22 23:10 OriolAbril

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!

pibieta avatar Oct 12 '22 22:10 pibieta