Skip to content

Use separate argument in CustomDist for functions that return symbolic representations #6462

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 1 commit into from
Jan 20, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jan 20, 2023

The initial API introduced in #6361 of overloading the random kwarg and branching depending on whether the function returned PyTensor variables or failed whatsoever was pretty bad. For instance it would mask obvious errors like calling pm.Normal.dist(banana=...).

Such errors won't be masked anymore, and the user must instead pass such functions to the dist kwarg. I added a temporary check to help users navigate the API change.

It also cleans some stale references to Aesara

@ricardoV94 ricardoV94 changed the title Use separate argument in CustomDist for functions that return symbolic representations Use separate argument in CustomDist for functions that return symbolic representations Jan 20, 2023
@ricardoV94 ricardoV94 force-pushed the customdist_symbolic_func branch from d45c662 to d45446b Compare January 20, 2023 10:55
@ricardoV94 ricardoV94 force-pushed the customdist_symbolic_func branch from d45446b to 320135e Compare January 20, 2023 10:58
@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Merging #6462 (320135e) into main (22b4446) will decrease coverage by 8.82%.
The diff coverage is 96.29%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6462      +/-   ##
==========================================
- Coverage   94.79%   85.97%   -8.82%     
==========================================
  Files         148      148              
  Lines       27716    27730      +14     
==========================================
- Hits        26272    23840    -2432     
- Misses       1444     3890    +2446     
Impacted Files Coverage Δ
pymc/distributions/distribution.py 96.60% <93.33%> (-0.87%) ⬇️
pymc/tests/distributions/test_distribution.py 98.72% <100.00%> (+0.02%) ⬆️
pymc/tests/logprob/test_utils.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_cumsum.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_mixture.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_abstract.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_rewriting.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_joint_logprob.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_composite_logprob.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/logprob/test_transforms.py 0.00% <0.00%> (-99.76%) ⬇️
... and 17 more

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

I wanted to suggest raising a deprecation warning, but old calls will now result in a TypeError because dist is a required kwarg.

Not great, but I guess there was the experimental warning, so YOLO.

@ricardoV94 ricardoV94 merged commit 00d2675 into pymc-devs:main Jan 20, 2023
@ricardoV94 ricardoV94 deleted the customdist_symbolic_func branch June 6, 2023 02:56
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.

2 participants