Deprecate redundant utilities for extracting constants #1046

ricardoV94 · 2024-10-21T16:08:16Z

Description

In PyTensor we have tho following similar utilities

pytensor.get_underlying_scalar_constant
pytensor.tensor.basic.get_underlying_scalar_constant_value
pytensor.tensor.basic.get_scalar_constant_value
pytensor.tensor.basic.extract_constant
pytensor.tensor.rewriting.math.get_constant

This PR removes and deprecates all except:

pytensor.tensor.basic.get_underlying_scalar_constant_value
pytensor.tensor.basic.get_scalar_constant_value

The reason for this distinction, is that the core utility, get_underlying_scalar_constant_value actually works for non-scalar inputs, if it can find a single scalar value underlies a potential n-dimensional tensor (say as in pt.zeros(5, 3, 2)). This is powerful, but can lead to subtle bugs when the caller forgets about it. This was the source of the bug behind #584 and was also likely present in other graphs such as gt(x, [0, 0, 0, 0]) and alike where the repeated condition broadcasts the output of the operation.

The utility get_scalar_constant_value raises if the input is not a scalar (ndim=0) type.

I don't love the underlying distinguishing the two. Perhaps unique would be better.

Both utilities now accept a raise_not_constant which when False (not-default) return the variable as is. I think I would prefer for it to return None but this requires less code changes.

Related Issue

Closes local_add_neg_to_sub rewrite gives wrong results with negative constants #584
Related to #

codecov · 2024-11-29T11:27:56Z

Codecov Report

Attention: Patch coverage is 86.66667% with 36 lines in your changes missing coverage. Please review.

Project coverage is 82.11%. Comparing base (8cc489b) to head (628c321).
Report is 183 commits behind head on main.

Files with missing lines	Patch %	Lines
pytensor/tensor/rewriting/math.py	75.25%	11 Missing and 13 partials ⚠️
pytensor/tensor/basic.py	88.00%	6 Missing ⚠️
pytensor/tensor/variable.py	85.71%	3 Missing ⚠️
pytensor/scan/rewriting.py	90.90%	0 Missing and 1 partial ⚠️
pytensor/tensor/rewriting/subtensor.py	80.00%	0 Missing and 1 partial ⚠️
pytensor/tensor/shape.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1046      +/-   ##
==========================================
- Coverage   82.14%   82.11%   -0.03%     
==========================================
  Files         186      186              
  Lines       48207    48201       -6     
  Branches     8677     8679       +2     
==========================================
- Hits        39598    39579      -19     
- Misses       6441     6447       +6     
- Partials     2168     2175       +7

Files with missing lines	Coverage Δ
pytensor/gradient.py	`77.62% <100.00%> (+0.05%)`	⬆️
pytensor/link/jax/dispatch/tensor_basic.py	`98.21% <100.00%> (ø)`
pytensor/scan/basic.py	`84.56% <100.00%> (ø)`
pytensor/sparse/basic.py	`82.57% <100.00%> (+0.02%)`	⬆️
pytensor/tensor/conv/abstract_conv.py	`76.05% <100.00%> (ø)`
pytensor/tensor/extra_ops.py	`88.01% <100.00%> (ø)`
pytensor/tensor/rewriting/basic.py	`94.24% <100.00%> (+0.09%)`	⬆️
pytensor/tensor/rewriting/elemwise.py	`91.13% <100.00%> (-0.02%)`	⬇️
pytensor/tensor/rewriting/shape.py	`82.02% <100.00%> (ø)`
pytensor/tensor/slinalg.py	`93.52% <100.00%> (+0.04%)`	⬆️
... and 7 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ricardoV94 · 2024-12-03T10:50:49Z

@Armavica I removed the negative constant rewrite in the last commit

jessegrabowski

Left some ticky-tacky comments, feel free to ignore.

One thing I noticed was that get_underlying_scalar_constant_value is available in pt.? We should maybe curate that namespace a bit more, because this isn't a function an average user is going to need, and use it from ptb directly.

jessegrabowski · 2025-01-12T12:07:20Z

pytensor/__init__.py

-        if v.owner is not None and isinstance(v.owner.op, sparse.CSM):
-            data = v.owner.inputs[0]
-            return tensor.get_underlying_scalar_constant_value(data)
+    warnings.warn(


Use the logger instead?

wdym. Never saw deprecation warnings in logging.

jessegrabowski · 2025-01-12T12:12:15Z

pytensor/gradient.py

    'maybe' means that x is an expression that is complicated enough
    that we can't tell that it simplifies to 0.
    """
+    from pytensor.tensor import get_underlying_scalar_constant_value


Why does this need a local import here but not in the above function?

Because the other is using it through tensor. This was not done on purpose but I prefer explicit imports and there is in fact a circular dependency here.

jessegrabowski · 2025-01-12T12:19:27Z

pytensor/tensor/basic.py

+    Parameters
+    ----------
+    v: Variable
+    elemwise : bool


check_through_elemwise_ops or something? Just elemwise makes it sound like its expecting an elemwise input

These were all stuff that existed. If I change the kwarg names it will be a breaking change which I think this PR is not

I guess I could keep the wrapper with the old kwargs and the inner with better but not sure that helps or makes things more confusing

jessegrabowski · 2025-01-12T12:20:49Z

pytensor/tensor/basic.py

+        If False, we won't try to go into elemwise. So this call is faster.
+        But we still investigate in Second Elemwise (as this is a substitute
+        for Alloc)
+    only_process_constants : bool


shallow_search? Why is "constants" plural?

jessegrabowski · 2025-01-12T12:22:07Z

pytensor/tensor/basic.py

+        underlying constant scalar value. If False, return `v` as is.
+
+
+    Raises


numpy docs suggest to only have a Raises section if its not obvious; I don't think its needed here

This is needed. It's actually how you use the function most of the time. You need to know what to try except

jessegrabowski · 2025-01-12T12:24:28Z

pytensor/tensor/basic.py

-    return x
+    warnings.warn(
+        "extract_constant is deprecated. Use `get_underlying_scalar_constant_value(..., raise_not_constant=False)`",
+        FutureWarning,


DeprecationWarning ?

DeprecationWarning is more invisible to users, so I always use FutureWarning

I see I used a DeprecationWarning above, I'll make it Future

jessegrabowski · 2025-01-12T12:57:11Z

pytensor/tensor/variable.py

-            if (flat_data == flat_data[0]).all():
-                return flat_data[0]
-
+    warnings.warn("get_unique_constant_value is deprecated.", FutureWarning)


DeprecationWarning ?

…ant_value`

…scalar

Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584

…constant

1. Use actual Solve Op to infer output dtype as CholSolve outputs a different dtype than basic Solve in Scipy==1.15 2. Tweaked test related to pymc-devs#1152 3. Tweak tolerage

ricardoV94 added the major label Oct 21, 2024

ricardoV94 changed the title ~~Remove repeated utilities for extracting constants~~ Deprecate redundant utilities for extracting constants Oct 21, 2024

ricardoV94 added bug Something isn't working maintenance graph rewriting labels Oct 21, 2024

ricardoV94 force-pushed the fix_rewrite_bug branch 7 times, most recently from 24716ac to 8dcbc2b Compare November 29, 2024 11:04

ricardoV94 marked this pull request as ready for review November 29, 2024 22:05

ricardoV94 requested a review from Armavica November 29, 2024 22:05

ricardoV94 force-pushed the fix_rewrite_bug branch from 8dcbc2b to 6c20b96 Compare December 3, 2024 10:50

ricardoV94 force-pushed the fix_rewrite_bug branch 2 times, most recently from 7961d57 to 8134f62 Compare December 9, 2024 11:45

ricardoV94 force-pushed the fix_rewrite_bug branch 3 times, most recently from 7c38bed to 15c06ff Compare January 10, 2025 18:37

ricardoV94 requested a review from jessegrabowski January 10, 2025 18:38

ricardoV94 force-pushed the fix_rewrite_bug branch from 15c06ff to c7712f6 Compare January 12, 2025 11:54

jessegrabowski approved these changes Jan 12, 2025

View reviewed changes

ricardoV94 added 5 commits January 13, 2025 13:10

Cache unique value of TensorConstants and deprecate `get_unique_const…

3b5b35c

…ant_value`

Deprecate pytensor.get_underlying_scalar_constant

b8e7783

Deprecate extract_constant

21523e7

Use more strict get_scalar_constant_value when the input must be a …

5b05713

…scalar

Remove internal get_constant helper

7738267

Fixes bug in `local_add_neg_to_sub` reported in pymc-devs#584

Do not apply local_add_neg_to_sub rewrite if negative variabe is a …

0b795af

…constant

ricardoV94 force-pushed the fix_rewrite_bug branch from c7712f6 to 65d4b7d Compare January 13, 2025 12:10

Adapt to Solve changes in Scipy 1.15

628c321

1. Use actual Solve Op to infer output dtype as CholSolve outputs a different dtype than basic Solve in Scipy==1.15 2. Tweaked test related to pymc-devs#1152 3. Tweak tolerage

ricardoV94 force-pushed the fix_rewrite_bug branch from 65d4b7d to 628c321 Compare January 13, 2025 14:19

ricardoV94 merged commit 581f65a into pymc-devs:main Jan 13, 2025
60 of 62 checks passed

ricardoV94 mentioned this pull request Jan 13, 2025

Deprecate get_undelying_scalar_constant_value from tensor namespace #1153

Open

		underlying constant scalar value. If False, return `v` as is.


		Raises

Deprecate redundant utilities for extracting constants #1046

Deprecate redundant utilities for extracting constants #1046

Uh oh!

Conversation

ricardoV94 commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Uh oh!

codecov bot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricardoV94 commented Dec 3, 2024

Uh oh!

jessegrabowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ricardoV94 commented Oct 21, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading