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.

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

Deprecate redundant utilities for extracting constants #1046

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

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Oct 21, 2024

Description

In PyTensor we have tho following similar utilities

  1. pytensor.get_underlying_scalar_constant
  2. pytensor.tensor.basic.get_underlying_scalar_constant_value
  3. pytensor.tensor.basic.get_scalar_constant_value
  4. pytensor.tensor.basic.extract_constant
  5. pytensor.tensor.rewriting.math.get_constant

This PR removes and deprecates all except:

  1. pytensor.tensor.basic.get_underlying_scalar_constant_value
  2. 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

@ricardoV94 ricardoV94 changed the title Remove repeated utilities for extracting constants Deprecate redundant utilities for extracting constants Oct 21, 2024
@ricardoV94 ricardoV94 force-pushed the fix_rewrite_bug branch 7 times, most recently from 24716ac to 8dcbc2b Compare November 29, 2024 11:04
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 86.99187% with 32 lines in your changes missing coverage. Please review.

Project coverage is 82.07%. Comparing base (0824dba) to head (8dcbc2b).

Files with missing lines Patch % Lines
pytensor/tensor/rewriting/math.py 74.02% 9 Missing and 11 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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1046      +/-   ##
==========================================
- Coverage   82.12%   82.07%   -0.05%     
==========================================
  Files         183      183              
  Lines       48111    48108       -3     
  Branches     8667     8665       -2     
==========================================
- Hits        39510    39485      -25     
- Misses       6435     6452      +17     
- Partials     2166     2171       +5     
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/subtensor.py 89.54% <100.00%> (ø)
... and 6 more

@ricardoV94 ricardoV94 marked this pull request as ready for review November 29, 2024 22:05
@ricardoV94 ricardoV94 requested a review from Armavica November 29, 2024 22:05
@ricardoV94
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

local_add_neg_to_sub rewrite gives wrong results with negative constants
1 participant