Skip to content

Rewrite useless log1mexp(log1mexp(x)) #474

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NathanielF
Copy link

@NathanielF NathanielF commented Oct 10, 2023

Closes #471

Small rewrite changes to allow pytensor to cancel iterative applications of the log1mexp() function

The lack of rewrite caused issues in the application of pm.Censored on the Weibull distribution: pymc-devs/pymc-examples#580

...

I've added a new case to the existing math re-write functions as discussed with @ricardoV94
...

Checklist

Major / Breaking Changes

NA

  • ...

New features

NA

  • ...

Bugfixes

NA

  • ...

Documentation

NA

  • ...

Maintenance

NA

@codecov-commenter
Copy link

Codecov Report

Merging #474 (8b5c55e) into main (36df379) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #474      +/-   ##
==========================================
- Coverage   80.66%   80.65%   -0.01%     
==========================================
  Files         160      160              
  Lines       46025    46030       +5     
  Branches    11266    11267       +1     
==========================================
  Hits        37124    37124              
- Misses       6668     6672       +4     
- Partials     2233     2234       +1     
Files Coverage Δ
pytensor/tensor/rewriting/math.py 86.19% <0.00%> (-0.28%) ⬇️

# Case for log1mexp(log1mexp(x)) -> x
if isinstance(prev_op, aes_math.Log1mexp) and isinstance(node_op, aes_math.Log1mexp):
if isinstance(prev_op, aes_math.Log1mexp) and isinstance(
Copy link
Member

@ricardoV94 ricardoV94 Oct 10, 2023

Choose a reason for hiding this comment

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

This makes more sense as a separate rewrite, analogous to the local_func_inv, something like local_func_inv_nan_switch. Otherwise looks nice. Would also need a test

@ricardoV94 ricardoV94 added enhancement New feature or request graph rewriting labels Oct 11, 2023
@ricardoV94 ricardoV94 changed the title [471] adding case for log1mexp(log1mexp(x)) -> x Rewrite useless log1mexp(log1mexp(x)) Oct 11, 2023
Signed-off-by: Nathaniel <[email protected]>
@NathanielF
Copy link
Author

Not entirely sure why this would fail @ricardoV94 , i haven't touched any code on the failing test:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request graph rewriting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize log1mexp(log1mexp(x)) -> x
3 participants