Fix dims coercion to list in model_from_fgraph

[williambdean]

add_named_variable is declared to take a tuple and is passed a list

pymc/model/core.py#L2196

def add_named_variable(self, var, dims: tuple[str | None, ...] | None = None):

pymc/model/fgraph.py#L381

-        dims = list(op.dims) if op.dims else None
+        dims = tuple(op.dims) if op.dims else None
         model.add_named_variable(var, dims=dims)

ModelVar coerces on the way in regardless, pymc/model/fgraph.py#L37, so the
graph never held a list:

self.dims = tuple(dims) if dims is not None else ()

Consequence: the round trip stops agreeing with itself

fgraph_from_model and model_from_fgraph are a pair, so every path built on
clone_model inherits the mismatch:

>>> model_from_fgraph(fgraph_from_model(m)[0]).named_vars_to_dims == m.named_vars_to_dims
False   # before
True    # after

Path	Before	After
Model.copy(), copy.copy, copy.deepcopy	False	True
pm.do	False	True
pm.observe	False	True

Invisible to CI: both files sit in the FAILING mypy baseline, scripts/run_mypy.py#L25.

No consumer breaks on a list

Reference	Behaviour
pymc/backends/arviz.py#L226	list(dvals), normalizes
pymc/backends/mcbackend.py#L57	list(...), normalizes
pymc/printing.py#L430	.get(), ignores type
pymc/variational/opvi.py#L1174	.get(), ignores type
pymc/model_graph.py#L355	in guard, then iterates
pymc/testing.py	assert_equivalent_model never reads dims

So this is a contract fix, not a crash fix.

The tests were the defect

Assertions hardcoded the round trip's output instead of comparing to the original:

assert m_new.named_vars_to_dims == {"z": ["test_dim"]}

Reference	Now
tests/model/test_fgraph.py#L65	== m_old.named_vars_to_dims
tests/model/transform/test_conditioning.py#L136	== m_old.named_vars_to_dims
tests/model/transform/test_conditioning.py#L234	== m.named_vars_to_dims
tests/model/transform/test_conditioning.py#L241	== m.named_vars_to_dims

Added tests/model/test_fgraph.py#L404, #L428, #L451, #L475, #L498,
#L520 for each isinstance branch, each dims spelling, clone_model, ModelVar
subclasses, and the unhandled fallthrough. 13 fail without the one-line change.

Incidental

pymc/model/fgraph.py#L378 raised TypeError naming the Variable, so it
reported a TensorVariable and said nothing about which Op was unrecognised.
Now names the Op.

Not fixed

pymc/model/fgraph.py#L258 reads .get(name, ()) while pymc/model/fgraph.py#L37
defaults dims=(), so omitted and explicit dims=() collapse in the graph.
Strict xfail at tests/model/test_fgraph.py#L545. Needs a sentinel or a second
prop on ModelVar, which touches __props__ and all five model_*_rv
constructors. Separately, else None to else () trades one silent loss for
another.

[read-the-docs-community]

Documentation build overview

📚 pymc | 🛠️ Build #34949462 | 📁 Comparing 0e663fc against latest (abaadf3)

  🔍 Preview build  

2 files changed

± glossary.html
± _modules/pymc/model/fgraph.html

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.95%. Comparing base (abaadf3) to head (f17b181).

Files with missing lines	Patch %	Lines
pymc/model/fgraph.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8465   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         128      128           
  Lines       21276    21276           
=======================================
  Hits        19564    19564           
  Misses       1712     1712           

Files with missing lines	Coverage Δ
pymc/model/fgraph.py	97.87% <50.00%> (ø)

🚀 New features to boost your workflow:

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

[ricardoV94]

I started and then stopped. My posterior is beta(1, 0), that all new tests are overkill, and changing the assertion on the original tests suffices. We can't add 20 tests everytime we fix something like list -> tuple ...

[ricardoV94]

overkill test

[ricardoV94]

overkill test

[ricardoV94]

overkill test

[williambdean]

Agreed. Cut it down to the fix plus the assertion changes: list(op.dims) -> tuple(op.dims), and the tests now compare to the original model instead of hardcoding ['test_dim']. All 8 new tests are gone, 13 insertions / 9 deletions.

Kept out as unrelated: the dims=() vs omitted dims xfail, since ModelVar cannot tell them apart and that is a different bug. Can open it separately.

Say the word if you want the adjacent type(model_var) -> type(op) change dropped too, for a strict single-character diff.

[williambdean]

Thanks for the review @ricardoV94 !