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

BUG: Using a length-only coord causes it to be volatile #7376

Open
JasonTam opened this issue Jun 19, 2024 · 5 comments · May be fixed by #7381
Open

BUG: Using a length-only coord causes it to be volatile #7376

JasonTam opened this issue Jun 19, 2024 · 5 comments · May be fixed by #7381
Labels

Comments

@JasonTam
Copy link
Contributor

JasonTam commented Jun 19, 2024

Describe the issue:

When a variable is defined with a coord, it's considered volatile. Then sample_posterior_predictive does not transfer the node over.
I believe the volatility condition that's met is

or ( # SharedVariables, except RandomState/Generators
isinstance(node, SharedVariable)
and not isinstance(node, RandomStateSharedVariable | RandomGeneratorSharedVariable)
and not shared_value_matches(node)
)

The coord is intended to be immutable, but after #7047, coords always mutable

Somewhat related:

Reproduceable code example:

import pymc as pm

y_obs = [0] * 3
with pm.Model() as m:
    m.add_coord("coord0", length=1)
    x = pm.Data("x", [0] * 3)
    b = pm.Normal("b", dims="coord0")  # Causes `b` to be volatile
    # b = pm.Normal("b", shape=1)      # Works as expected (`b` is transferred)
    y = pm.Normal("y", b[0] * x, observed=y_obs)
    idata = pm.sample()
    
    pm.sample_posterior_predictive(trace=idata, var_names='y')

Error message:

Sampling: [b, y]

PyMC version information:

5.15.1

Context for the issue:

Cannot use sample_posterior_predictive as intended

@JasonTam JasonTam added the bug label Jun 19, 2024
Copy link

welcome bot commented Jun 19, 2024

Welcome Banner]
🎉 Welcome to PyMC! 🎉 We're really excited to have your input into the project! 💖

If you haven't done so already, please make sure you check out our Contributing Guidelines and Code of Conduct.

@ricardoV94
Copy link
Member

The volatility logic is supposed to check if the values of mutable data actually changed (it's not enough to be mutable) so this is quite surprising

@JasonTam
Copy link
Contributor Author

Ah I think I understand what the issue is. The condition check for constant_coords is:

current_coord = model.coords.get(dim, None)
if (
current_coord is not None
and len(coord) == len(current_coord)
and np.all(coord == current_coord)
):
constant_coords.add(dim)

But if you add a coord without any values (just the length)

m.add_coord("coord0", length=1)

then the coord, will be None. We will only have dim_lengths information.

pymc/pymc/model/core.py

Lines 1030 to 1031 in 9eaf92a

self._dim_lengths[name] = length
self._coords[name] = values

I guess my question now: is this the intended?

@JasonTam JasonTam changed the title BUG: Using a coord causes basic RV to be volatile BUG: Using a length-only coord causes it to be volatile Jun 21, 2024
@ricardoV94
Copy link
Member

I see, we always always get tricked by coords without values (as in we tend to forget they exist all the time). In that case the volatility check should be whether the length changed or not?

What do you think? And do you want to open a PR to fix it?

@JasonTam
Copy link
Contributor Author

For sure it threw me in for a loop. So I think it should either be considered constant if the length stays the same, or a log message should be fired.

I've opened a PR #7381 and we can continue any discussion there

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 a pull request may close this issue.

2 participants