Skip to content

Implement dims CustomDist (alternative impl)#8346

Draft
ricardoV94 wants to merge 3 commits into
pymc-devs:mainfrom
ricardoV94:alt-customdist-dims
Draft

Implement dims CustomDist (alternative impl)#8346
ricardoV94 wants to merge 3 commits into
pymc-devs:mainfrom
ricardoV94:alt-customdist-dims

Conversation

@ricardoV94

@ricardoV94 ricardoV94 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Exploring alternative approach to #8311

Add a DimSymbolicRV analogous to SymbolicRV which we'll need for symbolic dims RV built out of other dims RV and primitives. Used by Censored and then by the new dims.CustomDist when dist is provided

@read-the-docs-community

read-the-docs-community Bot commented Jul 2, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.46809% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.80%. Comparing base (971800b) to head (978eba4).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
pymc/dims/distributions/core.py 90.24% 8 Missing ⚠️
pymc/dims/distributions/custom.py 96.33% 4 Missing ⚠️
pymc/dims/distributions/censored.py 97.29% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8346      +/-   ##
==========================================
+ Coverage   91.72%   91.80%   +0.07%     
==========================================
  Files         125      129       +4     
  Lines       20526    20855     +329     
==========================================
+ Hits        18828    19146     +318     
- Misses       1698     1709      +11     
Files with missing lines Coverage Δ
pymc/dims/distributions/__init__.py 100.00% <100.00%> (ø)
pymc/distributions/distribution.py 94.60% <100.00%> (+0.02%) ⬆️
pymc/dims/distributions/censored.py 97.67% <97.29%> (+4.57%) ⬆️
pymc/dims/distributions/custom.py 96.33% <96.33%> (ø)
pymc/dims/distributions/core.py 91.44% <90.24%> (-0.27%) ⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ricardoV94
ricardoV94 force-pushed the alt-customdist-dims branch from 971bd5b to 7a470a4 Compare July 2, 2026 20:44
Normalize size and convert params before the type check, and rebuild the
Op when the param types (not just the size type) don't match the inner
graph.
@ricardoV94
ricardoV94 force-pushed the alt-customdist-dims branch 2 times, most recently from 1c8004a to ecb679b Compare July 2, 2026 21:52
ricardoV94 and others added 2 commits July 2, 2026 23:55
…ions

The xtensor counterpart of SymbolicRandomVariable: an OpFromGraph that
dims distributions dispatch logp/logcdf/support_point on, with a single
piped RNG and a generic rebuild used to add extra dims. Censored is the
first user.
The dist function receives the params and extra_dims (the counterpart of
size) as XTensorVariables and returns an xtensor random graph, demarcated
by a DimCustomDistRV Op that logp/logcdf/support_point overrides are
dispatched on with xtensor semantics.

Co-authored-by: Will Dean <wd60622@gmail.com>
@ricardoV94
ricardoV94 force-pushed the alt-customdist-dims branch from ecb679b to 978eba4 Compare July 2, 2026 21:59
assert c3_dist.dims == ("d", "c", "a", "b")


def test_censored_custom_dist():

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really test custom dist not censored, and local imports -.-

},
)

# ---- Dispatch the user overrides with xtensor semantics ----

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ---- Dispatch the user overrides with xtensor semantics ----

@ricardoV94
ricardoV94 requested a review from williambdean July 5, 2026 08:31
)


class TestCustomDistSymbolic:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need this test class, there are no two forms of dist like in regular CustomDist

logp_val = model.compile_logp()(ip)
assert np.isfinite(logp_val)

def test_mu_as_model_var(self):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this testing with tensor logp? also way too many tests

@ricardoV94

ricardoV94 commented Jul 5, 2026

Copy link
Copy Markdown
Member Author

Still cleanup but feel free to take a look @williambdean

Not sure I landed very far from your proposal, but this is more or less what I imagined it may look like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant