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 for Max of continuous IID variables #6769

Merged
merged 18 commits into from
Jul 27, 2023

Conversation

Dhruvanshu-Joshi
Copy link
Member

@Dhruvanshu-Joshi Dhruvanshu-Joshi commented Jun 11, 2023

What is this PR about?
This PR aims to provide a solution for implementing the Max operation from order statistics to solve the issue [#6350] (#6350) and issue #6773. More tests are to be added.

Checklist

Major / Breaking Changes

  • ...

New features

  • Logprob derivation for pt.max

Bugfixes

  • ...

To-do:

  • Derive logprob for Max
  • Add tests to verify the logprob generated for max operation
  • Derive logprob for Min
  • Add tests to verify the logprob generated for min operation
  • Add support for different types of Distributions

Documentation

  • ...

Maintenance

  • ...

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

@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Merging #6769 (8bf0154) into main (510d3b8) will increase coverage by 2.90%.
The diff coverage is 97.95%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6769      +/-   ##
==========================================
+ Coverage   89.15%   92.05%   +2.90%     
==========================================
  Files          95       96       +1     
  Lines       16324    16373      +49     
==========================================
+ Hits        14553    15072     +519     
+ Misses       1771     1301     -470     
Files Changed Coverage Δ
pymc/logprob/order.py 97.82% <97.82%> (ø)
pymc/logprob/__init__.py 100.00% <100.00%> (ø)
pymc/logprob/rewriting.py 90.00% <100.00%> (+0.11%) ⬆️

... and 18 files with indirect coverage changes

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks like a great start, left some comments below.

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved


class MeasurableMax(MaxAndArgmax):
"""A placeholder used to specify a log-likelihood for a clipped RV sub-graph."""
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
"""A placeholder used to specify a log-likelihood for a clipped RV sub-graph."""
"""A placeholder used to specify a log-likelihood for a max sub-graph."""

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Great progress @Dhruvanshu-Joshi! Some comments to complement @ricardoV94's review

pymc/logprob/order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_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
Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Great progress @Dhruvanshu-Joshi! Slight comments about the documentation on my side

docs/source/api/order_stats.rst 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/order.py Outdated Show resolved Hide resolved
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks good. Let's separate the docs into their own PR.

You can use an already supported case like cumsum instead of the max in that PR

tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
docs/source/api/logprob.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
docs/source/api/order_stats.rst Outdated Show resolved Hide resolved
Copy link
Member

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Some comments in preparation for tomorrow's meeting :) This PR is shaping up very nicely, we're close to merging!

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
tests/logprob/test_order.py Outdated Show resolved Hide resolved
@Dhruvanshu-Joshi Dhruvanshu-Joshi marked this pull request as ready for review July 14, 2023 21:07
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Everything looks pretty good, just a small check missing I think.

pymc/logprob/order.py Outdated Show resolved Hide resolved
pymc/logprob/order.py Outdated Show resolved Hide resolved
ricardoV94
ricardoV94 previously approved these changes Jul 17, 2023
@Dhruvanshu-Joshi Dhruvanshu-Joshi mentioned this pull request Jul 23, 2023
5 tasks
@larryshamalama
Copy link
Member

Remind me what's pending for merging this PR?

@ricardoV94
Copy link
Member

I think it just needs to be rebased so the tests pass. There was an unrelated failure that crop up elsewhere

@ricardoV94 ricardoV94 merged commit e3961fc into pymc-devs:main Jul 27, 2023
21 checks passed
@ricardoV94 ricardoV94 changed the title Logprob derivation for Max Logprob derivation for Max of continuous IID variables Jul 27, 2023
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