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.

Sign up for GitHub

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

Jump to bottom

local_add_neg_to_sub rewrite gives wrong results with negative constants #584

Open
ricardoV94 opened this issue Jan 11, 2024 · 2 comments · May be fixed by #1046
Open

local_add_neg_to_sub rewrite gives wrong results with negative constants #584

ricardoV94 opened this issue Jan 11, 2024 · 2 comments · May be fixed by #1046
Labels
bug Something isn't working graph rewriting

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Jan 11, 2024 •
edited
Loading

Reported in https://discourse.pymc.io/t/shape-issue-with-custom-logp-in-densitydist/13608/5?u=ricardov94

import numpy as np

import pytensor
import pytensor.tensor as pt

x = pt.scalar("x")
neg_const = pt.as_tensor(np.full((10,), -0.1))
out = neg_const + x 

pytensor.function([x], out)
<<!! BUG IN FGRAPH.REPLACE OR A LISTENER !!>> <class 'TypeError'> The type of the replacement (Vector(float64, shape=(1,))) must be compatible with the type of the original Variable (Vector(float64, shape=(10,))). local_add_neg_to_sub
ERROR (pytensor.graph.rewriting.basic): Rewrite failure due to: local_add_neg_to_sub
ERROR (pytensor.graph.rewriting.basic): node: Add([-0.1 -0.1 ... -0.1 -0.1], ExpandDims{axis=0}.0)
ERROR (pytensor.graph.rewriting.basic): TRACEBACK:
ERROR (pytensor.graph.rewriting.basic): Traceback (most recent call last):
  File "/home/ricardo/Documents/Projects/pytensor/pytensor/graph/rewriting/basic.py", line 1968, in process_node
    fgraph.replace_all_validate_remove(  # type: ignore
  File "/home/ricardo/Documents/Projects/pytensor/pytensor/graph/features.py", line 626, in replace_all_validate_remove
    chk = fgraph.replace_all_validate(replacements, reason=reason, **kwargs)
  File "/home/ricardo/Documents/Projects/pytensor/pytensor/graph/features.py", line 571, in replace_all_validate
    fgraph.replace(r, new_r, reason=reason, verbose=False, **kwargs)
  File "/home/ricardo/Documents/Projects/pytensor/pytensor/graph/fg.py", line 508, in replace
    self.change_node_input(
  File "/home/ricardo/Documents/Projects/pytensor/pytensor/graph/fg.py", line 419, in change_node_input
    raise TypeError(
TypeError: The type of the replacement (Vector(float64, shape=(1,))) must be compatible with the type of the original Variable (Vector(float64, shape=(10,))).

pytensor/pytensor/tensor/rewriting/math.py

Lines 1897 to 1922 in c52154d

@register_specialize
@node_rewriter([add])
def local_add_neg_to_sub(fgraph, node):
"""
-x + y -> y - x
x + (-y) -> x - y
"""
# This rewrite is only registered during specialization, because the
# `local_neg_to_mul` rewrite modifies the relevant pattern during canonicalization
# Rewrite is only applicable when there are two inputs to add
if node.op == add and len(node.inputs) == 2:
# Look for pattern with either input order
for first, second in (node.inputs, reversed(node.inputs)):
if second.owner:
if second.owner.op == neg:
pre_neg = second.owner.inputs[0]
new_out = sub(first, pre_neg)
return [new_out]
# Check if it is a negative constant
const = get_constant(second)
if const is not None and const < 0:
new_out = sub(first, np.abs(const))
return [new_out]

The helper get_constant actually calls get_unique_constant_value under the hood, which in this case returns a single scalar, not the filled homogenous vector. The rewrite itself is also expecting only a scalar constant, otherwise it should be using np.all
@ricardoV94 ricardoV94 added bug Something isn't working graph rewriting labels Jan 11, 2024
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Oct 21, 2024
@ricardoV94
Remove internal get_constant helper
1461a39 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
@ricardoV94 ricardoV94 linked a pull request Oct 21, 2024 that will close this issue
Open
2 tasks
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
7d68985 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
9177f1b 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
c585f88 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
30f5b3d 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
a336fb9 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 28, 2024
@ricardoV94
Remove internal get_constant helper
24716ac 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Nov 29, 2024
@ricardoV94
Remove internal get_constant helper
8dcbc2b 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
@Armavica
Copy link
Member

pytensor/pytensor/tensor/rewriting/math.py

Lines 1918 to 1922 in c52154d

# Check if it is a negative constant
const = get_constant(second)
if const is not None and const < 0:
new_out = sub(first, np.abs(const))
return [new_out]

Do we actually want to keep this part? If it replaces x + neg_const with x - pos_const, is this actually useful?
Plus, doesn't this prevent things such as -x + -6 from being rewritten as -6 - x (actually saving an op here)?

@ricardoV94
Copy link
Member Author

Good point, it doesn't save us anything with a constant. Only reason would be for canonicalization but this is a specialization rewrite.

Will remove that branch and see if anything was relying on it

ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Dec 3, 2024
@ricardoV94
Remove internal get_constant helper
f75d89d 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
ricardoV94 added a commit to ricardoV94/pytensor that referenced this issue Dec 9, 2024
@ricardoV94
Remove internal get_constant helper
a7e08e5 
Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working graph rewriting
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Deprecate redundant utilities for extracting constants ricardoV94/pytensor
2 participants
@Armavica @ricardoV94