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

Logprob derivation of Min for Discrete IID distributions #6968

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

Dhruvanshu-Joshi
Copy link
Member

@Dhruvanshu-Joshi Dhruvanshu-Joshi commented Oct 24, 2023

What is this PR about?
This PR is the extension of PR 6790 and aims to provide a solution for implementing the Min operation for discrete distributions from order statistics to solve the issue 6350 and issue 6773.

CC: @larryshamalama @ricardoV94

Checklist

Major / Breaking Changes

  • ...

New features

  • ...

Bugfixes

  • ...

Documentation

  • ...

Maintenance

  • ...

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

@ricardoV94 ricardoV94 changed the title Dev min discrete Logprob derivation of Min for Discrete IID distributions Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7bb2ccd) 92.21% compared to head (829b63c) 83.57%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6968      +/-   ##
==========================================
- Coverage   92.21%   83.57%   -8.64%     
==========================================
  Files         101      101              
  Lines       16912    16937      +25     
==========================================
- Hits        15595    14155    -1440     
- Misses       1317     2782    +1465     
Files Coverage Δ
pymc/logprob/order.py 98.13% <95.23%> (+2.80%) ⬆️
pymc/logprob/transforms.py 95.73% <90.00%> (-0.15%) ⬇️
pymc/logprob/utils.py 97.20% <94.11%> (-0.42%) ⬇️

... and 19 files with indirect coverage changes

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Show resolved Hide resolved
@ricardoV94
Copy link
Member

ricardoV94 commented Nov 2, 2023

Maybe a good test for discrete would be min/max(Bernoulli)?

def test_min_max_bernoulli():
    p = 0.7
    q = 1 - p
    n = 3
    x = pm.Bernoulli.dist(p=p, shape=(n,))
    value = pt.scalar("value", dtype=int)

    max_logp_fn = pytensor.function([value], pm.logp(pt.max(x), value))
    np.testing.assert_allclose(max_logp_fn(0), np.log(q ** n))
    np.testing.assert_allclose(max_logp_fn(1), np.log(1 - q ** n))

    min_logp_fn = pytensor.function([value], pm.logp(pt.min(x), value))
    np.testing.assert_allclose(min_logp_fn(1), np.log(p ** n))
    np.testing.assert_allclose(min_logp_fn(0), np.log(1 - p ** n))

This suggests @larryshamalama implementation is correct. I guess I was tripped by the cdf for discrete variables being P(X<=x) and not P(X<x)

pymc/logprob/transforms.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Outdated Show resolved Hide resolved
@Dhruvanshu-Joshi Dhruvanshu-Joshi force-pushed the dev_min_discrete branch 2 times, most recently from c885c32 to 4d2ac0d Compare November 7, 2023 13:04
@Dhruvanshu-Joshi
Copy link
Member Author

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
@Dhruvanshu-Joshi
Copy link
Member Author

Todo:
Once this is merged, I'll open a seperate PR so that the test for transforms that fails does not do so.

@Dhruvanshu-Joshi Dhruvanshu-Joshi force-pushed the dev_min_discrete branch 2 times, most recently from a2ef7a7 to 69f1b32 Compare November 26, 2023 05:34
@Dhruvanshu-Joshi
Copy link
Member Author

Hey @ricardoV94 @larryshamalama . I have commited the changes discussed in the last week. Please give it a look as you find the time.

pymc/logprob/utils.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Outdated Show resolved Hide resolved
pymc/logprob/transforms.py Outdated Show resolved Hide resolved


measurable_ir_rewrites_db.register(
"find_measurable_max_neg",
find_measurable_max_neg,
"basic",
"min",
"min_discrete",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"min_discrete",
"min",

@ricardoV94 ricardoV94 force-pushed the dev_min_discrete branch 3 times, most recently from 416d3ac to d97d0af Compare January 9, 2024 14:51
@ricardoV94
Copy link
Member

Great work @Dhruvanshu-Joshi

@ricardoV94 ricardoV94 merged commit 6554683 into pymc-devs:main Feb 5, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants