Skip to content

Remove workaround in InferenceData conversion; require ArviZ >=0.11.4 #5060

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 2 commits into from
Oct 11, 2021

Conversation

michaelosthege
Copy link
Member

This PR removes a workaround in the InferenceData conversion and at the same time fixes a bug that was present in the workaround.

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

  • what are the (breaking) changes that this PR makes? → possibly ArviZ API changes
  • important background, or details about the implementation →
  • are the changes—especially new features—covered by tests and docstrings? → yup
  • linting/style checks have been run → aye

@@ -160,7 +120,6 @@ def __init__(
model=None,
save_warmup: Optional[bool] = None,
density_dist_obs: bool = True,
Copy link
Member

Choose a reason for hiding this comment

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

doesn't need to be in this PR, but we can remove this argument. densitydist api will be fixed in v4 so this is no longer necessary.

@codecov
Copy link

codecov bot commented Oct 10, 2021

Codecov Report

Merging #5060 (b18a09e) into main (33dd132) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5060      +/-   ##
==========================================
- Coverage   78.42%   78.41%   -0.01%     
==========================================
  Files         130      130              
  Lines       24475    24461      -14     
==========================================
- Hits        19195    19182      -13     
+ Misses       5280     5279       -1     
Impacted Files Coverage Δ
pymc/backends/arviz.py 88.75% <100.00%> (-0.60%) ⬇️
pymc/tests/test_distributions.py 96.34% <100.00%> (+0.07%) ⬆️

@michaelosthege
Copy link
Member Author

thanks for dealing with the dependencies, @OriolAbril !

I just rebased on main to resolve conflicts. Also I rearranged the commits such that the dependency upgrade is now all in one. Let's hope the CI likes it.

@michaelosthege
Copy link
Member Author

@OriolAbril can you check the diff again? I had to mark a test as XFAIL because I couldn't reproduce the float64/float32 casting error locally.
My feeling is that this is not critical, and because it's a good thing to have less strict dependencies we can go ahead?

OriolAbril and others added 2 commits October 11, 2021 09:59
The changes from 33dd132
(probably unpinning NumPy) fixed `test_wald_logcdf`
and broke `test_negative_binomial`.
@twiecki
Copy link
Member

twiecki commented Oct 11, 2021

Can merge once we get green light.

@michaelosthege michaelosthege merged commit 9dcd216 into pymc-devs:main Oct 11, 2021
@michaelosthege michaelosthege deleted the fix-5042 branch October 11, 2021 08:53
@twiecki
Copy link
Member

twiecki commented Oct 11, 2021

👍

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