Support symbolic eta parameter in LKJCholeskyCov.logp

Development
1

Hi everyone,

I came across an open TODO in pymc/distributions/multivariate.py (around line 1297) for _LKJCholeskyCovRV_logp.

# TODO: _lkj_normalizing_constant currently requires `eta` and `n` to be constants
try:
    eta = float(get_underlying_scalar_constant_value(eta))
except NotScalarConstantError:
    raise NotImplementedError("logp only implemented for constant `eta`")

Currently, this raises a NotImplementedError whenever a user passes a symbolic tensor or prior (such as pm.HalfNormal) to eta. However, _lkj_normalizing_constant already relies on symbolic PyTensor operations (gammaln, pt.switch, pt.log), and LKJCorr.logp already supports dynamic eta while keeping n constant.

Should I make a PR lifting this scalar constant constraint and adding unit tests for symbolic eta support in test_multivariate.py?

Thanks!

2

Do you have a specific model where this is blocking you?

3

It isn’t blocking a personal model of mine right now. I was exploring the codebase for open TODOs and areas to contribute, and I came across the NotImplementedError restriction on eta in _LKJCholeskyCovRV_logp.

The motivation is bringing LKJCholeskyCov into feature parity with LKJCorr (which already supports dynamic eta). In hierarchical modeling, users occasionally want to infer eta by putting a prior on it (for example, in this discussion), and currently LKJCholeskyCov raises an error if eta is anything other than a static float constant.

I mainly wanted to check with the team first before opening an unprompted PR about whether this change was worth making. If this is something you’d like brought in for consistency, I’m happy to put up a PR, otherwise, no worries at all!

4

(post deleted by author)