Skip to content

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 12, 2023

This PR simplifies the examples and explanations used in the Potential docstrings


📚 Documentation preview 📚: https://pymc--6772.org.readthedocs.build/en/6772/

@ricardoV94 ricardoV94 force-pushed the simplify_potential_docstrings branch from 61bb9dc to 515f7ab Compare June 12, 2023 07:28
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #6772 (515f7ab) into main (154f5b0) will decrease coverage by 0.46%.
The diff coverage is 100.00%.

❗ Current head 515f7ab differs from pull request most recent head a71fa6a. Consider uploading reports for the commit a71fa6a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6772      +/-   ##
==========================================
- Coverage   91.89%   91.44%   -0.46%     
==========================================
  Files          95       95              
  Lines       16147    16147              
==========================================
- Hits        14839    14765      -74     
- Misses       1308     1382      +74     
Impacted Files Coverage Δ
pymc/model.py 89.66% <100.00%> (-0.25%) ⬇️

... and 4 files with indirect coverage changes

@daniel-saunders-phil
Copy link
Contributor

Hey Ricardo, is the first example is supposed to be executable?

data = stats.norm(0.05,1).rvs(50)

with pm.Model() as m8:
    x = pm.Normal("x", mu=0, sigma=1)
    y = pm.Normal("y", mu=x, sigma=1, observed=data)
    constraint = x >= 0
    potential = pm.Potential("x_constraint", pm.math.log(pm.math.switch(constraint, 1, 0)))
    
    t8 = pm.sample()

I get AttributeError: 'float' object has no attribute 'type' . Full error trace back: https://gist.github.com/daniel-saunders-phil/b6656f51b0fae351274711dcfb2641f2 . The other examples under Potential work fine. Also, if I change the switch from a hard constraint to a very firm constraint (pm.math.switch(constraint, 1, 0.1)), the example runs (with poor sampler performance). So it looks like the problem is unique to adding log of 0 to the potential.

@ricardoV94
Copy link
Member Author

I think this was fixed in the very last release of PyTensor (1 or 2 days old) and I was probably already on it.

Can you update and check again?

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 13, 2023

By the way I think you're looking at the old example (not that it matters for the error your stumbled upon)

Also these switch Potential models are not expected to sample well, it's just for illustration. The discontinuities they introduce are pretty bad for NUTS

@daniel-saunders-phil
Copy link
Contributor

I think this was fixed in the very last release of PyTensor (1 or 2 days old) and I was probably already on it.

Can you update and check again?

Ahh okay you're on top of it. Doesn't look the latest version is available through conda-forge at the moment. I can only get 2.12.1. The new examples are also much nicer to read btw

Co-authored-by: Bill Engels <[email protected]>
@ricardoV94 ricardoV94 merged commit e69dec9 into pymc-devs:main Jun 16, 2023
@ricardoV94 ricardoV94 deleted the simplify_potential_docstrings branch September 18, 2023 14:30
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.

4 participants