Rewrite log(sqr(x)) to 2 * log(abs(x)) to avoid underflow - #2380
Conversation
A squared product underflows to zero in float32 at a few hundred elements, sending log(abs(sqr(prod(x)))) to -inf; unwrapping the square lets the existing log(abs(prod(x))) -> sum(log(abs(x))) case fire before the product is ever materialized.
| @register_canonicalize | ||
| @register_specialize | ||
| @node_rewriter([pt_abs]) | ||
| def local_abs_sqr(fgraph, node): |
There was a problem hiding this comment.
Include any scalar op that is non negative? Also use the new syntax x.owner_op_inputs?
There was a problem hiding this comment.
I added a non_negative flag on ScalarOp just for the occasion!
| @register_stabilize | ||
| @register_specialize | ||
| @node_rewriter([log]) | ||
| def local_log_sqr(fgraph, node): |
There was a problem hiding this comment.
extend the sqrt rewrite instead? it's the same concept?
| pytest.param(lambda x: pt_abs(sqr(x)), id="abs_sqr"), | ||
| ], | ||
| ) | ||
| def test_sqr_rewrites_skip_complex(original_fn): |
There was a problem hiding this comment.
tell your bot about RewriteTest or whatever it's called...
There was a problem hiding this comment.
: )
I love when people talk about how "we're going to have a tool that learns over time" as if these things aren't totally blank slate every time you boot them up.
Sharing the tail means sharing the constant's dtype, which is what stops the 0.5 factor from truncating to 0 on integer input. Closes pymc-devs#2379
|
|
||
| x_test = np.array([-1e3, -2.5, -1.0, -0.0, 0.0, 1e-8, 1.0, 2.5, 1e3]) | ||
| for op in flagged: | ||
| x = pt.vector("x") |
There was a problem hiding this comment.
Not really a scalar test... Also do we test monotonicity? seems like over-zealous a bit?
FunctionGraph.replace filters the replacement through the old variable's type, so a rewrite that changed dtype would be rejected outright and these could never fail.
The hazard it covered is still pinned by test_useless_abs_keeps_signed_zero, which fails if Sqrt is ever flagged.
| def local_log_sqrt(fgraph, node): | ||
| x = node.inputs[0] | ||
| @node_rewriter([pt_abs]) | ||
| def local_useless_abs(fgraph, node): |
There was a problem hiding this comment.
does this supersede some other pre-existing more narrow rewrite?
There was a problem hiding this comment.
no, but local_log_prod_to_sum_log was hand-rolling the check for non-negative Ops. I switched it to use _is_provably_non_negative (and by extension the new tag)
| return [new_out] | ||
|
|
||
|
|
||
| def _is_non_negative(var) -> bool: |
There was a problem hiding this comment.
we have a smarter is non negative somewhere else (used for subtensor lift rewrites maybe)? that we should repurpose/extend with this? It includes stuff like dtype or dimshuffle / ravel ... bla bla
There was a problem hiding this comment.
We have _is_provably_non_negative but it doesn't have any logic about functions like exp/softplus/whatever. I could extend it but I thought that was overkill
There was a problem hiding this comment.
I added a case to _is_provably_non_negative that uses the new non_negative flag, then deleted the duplicate is_negative helper
The flag case is non-strict only, since abs and sqr attain zero, and is restricted to float and unsigned outputs because signed integers wrap on overflow.
Nothing in pytensor or pymc ever wrote the tag.positive hook the removed branch read, and the predicate narrows signed-integer operands, where sqr can wrap negative.
| scalar_op = op.scalar_op | ||
| if not strict and isinstance(scalar_op, Cast): | ||
| return _is_provably_positive(var.owner.inputs[0], strict) | ||
| if ( |
There was a problem hiding this comment.
follow up issue to move this outside of rewriting/subtensor.py
ricardoV94
left a comment
There was a problem hiding this comment.
Tests seem a bit overkill since the last iteration, but ok...
When a rewrite correctly does not fire, assert_eval compiles the unchanged graph twice and compares it to itself, so the int8 test now pins the actual wrapped output instead.
|
I trimmed back the tests, will merge when CI passes again. |
Taking the log of a squared product materializes the product first, and in float32 a product of a few hundred terms underflows to zero once squared, so
log(abs(sqr(prod(x))))returns-inf. Shows up in GP code with a few hundred inducing points, on every backend.log(sqrt(x))andlog(sqr(x))are now one rewrite, so the square peels off and the existinglog(abs(prod(x))) -> sum(log(abs(x)))case finishes the job before anything is squared. Dropping theabsis driven by a newnon_negativeflag on unary scalar ops instead of a hardcoded list, soabs(exp(x))folds too.Sqrtdeliberately doesn't carry the flag:sqrt(-0.0)is-0.0, and dropping theabsthere would flip the sign of a downstream division.Closes #2379. Sharing one rewrite means sharing the constant's dtype, which is what stops the
0.5factor truncating to0on integer input.