pymc-devs / pymc-devs/pytensor

Constant.signature() is broken or missing for several subclasses

Open
#2,044 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug graph objects
Dominant language
Python
Stars
644
Forks
208
Avg merge
2d 14h
Merged PRs (30d)
16

Description

Description

Constant.signature() is the contract used for value-based equality and hashing (used by merge rewrites via merge_signature(), AtomicVariable.equals(), and FrozenApply interning). Several Constant subclasses have broken or missing implementations.

Problems found

  1. ScalarConstant had no signature() override (fixed in #2043)

It inherited the base Constant.signature() which returns (self.type, self.data). For NaN values, this breaks after pickle round-trip because nan != nan, causing C
module cache key lookup failures.

  1. TypedListConstant.signature() crashes

The base Constant.signature() returns (self.type, self.data) where data is a Python list of arrays. Both __eq__ and __hash__ are broken:

  >>> c1.signature() == c2.signature()
  # ValueError: The truth value of an array with more than one element is ambiguous.                                                                                     
  >>> hash(c1.signature())                                          
  # TypeError: unhashable type: 'list'                                                                                                                                   
  1. Bare Constant() instantiations bypass subclass signatures

Several places create Constant(type, data) directly instead of the appropriate subclass:

  • Constant(slicetype, slice(None)) in type_other.py (should be SliceConstant)
  • Constant(generic, None) in sparse/basic.py
  • Constant(node.params_type, params) in link/c/basic.py

This is a bigger problem of:

  1. Variables can bypass proper subclass constructors

As noted in pytensor/scalar/basic.py L1000-1001:

  # NOTE: We may have a Variable with ScalarType that is not ScalarVariable/ScalarConstant/ScalarSharedVariable
  # if it bypasses those constructs (same issue with TensorVariables). We should make that impossible to happen!

A Constant with ScalarType that isn't a ScalarConstant would use the broken base signature.

We e shouldn't ever create Variable(type=ScalarType|TensorType|...) that is not a ScalarVariable|TensorVariable. Code may handle it wrongly depending on whether it checks for isinstance(x, TensorVariable) or isinstance(x.type, TensorType).

Potential solutions

  • Add proper signature() to TypedListConstant
  • Replace bare Constant() calls with appropriate subclasses
  • Consider enforcing Type.constant_type at construction time so Constant(some_type, data) dispatches to the right subclass (factory new)
  • Consider making Constant.signature() abstract (currently blocked by bare Constant() usages)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with Constant.signature() and its uses in merge_signature(), AtomicVariable.equals(), and FrozenApply interning. Inspect the bare Constant constructions in type_other.py, sparse/basic.py, and link/c/basic.py, along with the constructor concern at pytensor/scalar/basic.py lines 1000-1001. Done means affected Constant subclasses have reliable equality and hashing without the reported crashes or round-trip failures.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.