-
Notifications
You must be signed in to change notification settings - Fork 135
Cover more cases of log1mexp
stabilization
#1483
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a question about readability at the cost of performance. We can merge as is if you disagree
log1pmexp_to_log1mexp
using is_neg
(fixes #1476)log1pmexp_to_log1mexp
using is_neg
to cover more cases
…g through the clients
I am not sure if you noticed, there is an opportunity to implement |
Yeah I think we can merge them, they're similar in nature |
I think it's ready to go. |
log1pmexp_to_log1mexp
using is_neg
to cover more caseslog1mexp
stabilization
Great, running tests |
Ops, I think found a mistake. I'll commit asap. |
…r not before proceeding
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (95.45%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1483 +/- ##
==========================================
+ Coverage 82.00% 82.01% +0.01%
==========================================
Files 214 214
Lines 50413 50431 +18
Branches 8902 8907 +5
==========================================
+ Hits 41342 41362 +20
+ Misses 6863 6861 -2
Partials 2208 2208
🚀 New features to boost your workflow:
|
Thanks @lciti ! Let us know if you find more things to tweak |
Many thanks to you for your support and, more in general, for developing/maintaining such an invaluable research tool! |
This PR fixes #1476 by reimplementing the rewrite
log1pmexp_to_log1mexp
usingis_neg
so it works withneg()
as well asmul(-1.0, ...)
, which is the canonicalised form. It adds a test for #1476 (which now passes) as well as other tests for expressions that should simplify tolog1mexp
(I realised that expressions likelog(-exp(x) + 1)
would not work). It then implements the missing rewritelog(-expm1(x)) -> log1mexp(x)
to make these additional tests pass.Description
Related Issue
log1pmexp_to_log1mexp
is not applied #1476Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1483.org.readthedocs.build/en/1483/