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

Add pickle test and revert theano var names (hopefully temporarily) #4117

Closed
wants to merge 7 commits into from

Conversation

canyon289
Copy link
Member

@canyon289 canyon289 commented Sep 18, 2020

This commit does a couple of things per comments in #4112

  1. Adding tests to cover this issue
  2. "Comment out" reverts changes in adding meaningful str representations to PyMC3 objects #4076

Proposed steps, pending successful CI, is that we merge this to fix broken SMC parallel capability and ArviZ pipelines, and then try reimplementing this feature in a way that doesn't break pickling of pm.Deterministic

@canyon289 canyon289 changed the title [WIP] Add eight schools pickle test [WIP] Add pickle test Sep 18, 2020
@codecov
Copy link

codecov bot commented Sep 19, 2020

Codecov Report

Merging #4117 into master will decrease coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4117      +/-   ##
==========================================
- Coverage   88.75%   88.46%   -0.29%     
==========================================
  Files          89       89              
  Lines       14040    14028      -12     
==========================================
- Hits        12461    12410      -51     
- Misses       1579     1618      +39     
Impacted Files Coverage Δ
pymc3/distributions/distribution.py 92.80% <ø> (-1.63%) ⬇️
pymc3/model.py 88.07% <ø> (-1.26%) ⬇️
pymc3/util.py 91.86% <100.00%> (-2.53%) ⬇️
pymc3/tests/models.py 79.13% <0.00%> (-8.64%) ⬇️
pymc3/step_methods/hmc/base_hmc.py 95.00% <0.00%> (-2.50%) ⬇️
pymc3/distributions/continuous.py 92.35% <0.00%> (-0.58%) ⬇️
pymc3/distributions/multivariate.py 80.78% <0.00%> (-0.15%) ⬇️

@canyon289 canyon289 changed the title [WIP] Add pickle test Add pickle test Sep 19, 2020
@canyon289
Copy link
Member Author

This is ready for review, I guess our code coverage tolerance is set pretty high hence the CI failure

@canyon289 canyon289 changed the title Add pickle test Add pickle test and revert theano var names (hopefully temporarily) Sep 19, 2020
@AlexAndorra
Copy link
Contributor

Pinging @Spaak, as I did in the associated issue

@canyon289
Copy link
Member Author

Sounds good, going to ask if we can merge in 24 hours if no strong objection is raised, as this right now put ArviZ at risk

@Spaak
Copy link
Member

Spaak commented Sep 20, 2020

Thanks for the ping also here, and for the tests. The issue appears to be with the "type hacking" I added to Deterministic in order to force str override to work with non-class-based types, and thankfully not with the new string machinery in general. I'll file a PR for a more targeted fix in a few hours.

@MarcoGorelli
Copy link
Contributor

@Spaak do you have a test which fails if you just do

    var.__str__ = functools.partial(_repr_deterministic_rv, var, formatting='plain')

? For me, TestStrAndLatexRepr from pymc3/tests/test_distributions.py passes like this:

$ pytest pymc3/tests/test_distributions.py -k TestStrAndLatexRepr -q
...                                                                                                                                                                                                [100%]
============================================================================================ warnings summary ============================================================================================
pymc3/tests/test_distributions.py::TestStrAndLatexRepr::test__repr_latex_
  /home/marco/miniconda/envs/pymc3-dev/lib/python3.8/site-packages/theano/tensor/var.py:468: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.9 it will stop working
    isinstance(args_el, collections.Iterable)):

pymc3/tests/test_distributions.py: 48 warnings
  /home/marco/pymc3-dev/pymc3/util.py:149: DeprecationWarning: np.asscalar(a) is deprecated since NumPy v1.16, use a.item() instead
    return asscalar(value)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
3 passed, 144 deselected, 49 warnings in 2.52s

@Spaak
Copy link
Member

Spaak commented Sep 20, 2020

@Spaak do you have a test which fails if you just do

    var.__str__ = functools.partial(_repr_deterministic_rv, var, formatting='plain')

?

@MarcoGorelli Yes, I do now, see #4120 . Patching __str__ like that on the fly was the first thing I tried, but unfortunately Python does not always use an instance's obj.__str__() when str(obj) is called, but instead does some other type of lookup, which is why I had to add the type-patching. See also https://stackoverflow.com/a/5918210/1692028

@canyon289
Copy link
Member Author

Thanks @Spaak!!

@canyon289 canyon289 closed this Sep 20, 2020
@canyon289 canyon289 deleted the pickle_test branch January 30, 2022 18:36
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.

4 participants