Do not consider dims without coords volatile if length has not changed #7381
Conversation
|
The main part I feel uneasy about is having to call |
pymc/sampling/forward.py
Outdated
| if hasattr(current_length, 'eval'): | ||
| current_length = current_length.eval() |
There was a problem hiding this comment.
It can only be a shared variable in which case you can retrieve with get_value(), or a Constant in which case you can retrieve with .data That should be much more efficient than eval unless eval does something clever under the hood
|
Also we need a test @JasonTam |
ricardoV94
changed the title
|
@ricardoV94 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7381 +/- ##
==========================================
- Coverage 92.18% 92.18% -0.01%
==========================================
Files 103 103
Lines 17249 17258 +9
==========================================
+ Hits 15901 15909 +8
- Misses 1348 1349 +1
|
|
@JasonTam here is where we test this volatility logic: pymc/tests/sampling/test_forward.py Line 113 in cbf1591 Can you add something there instead. No need to do actual sampling, just make a scenario where a variable depends on a dim without coords (and then change it's length or not) |
|
@ricardoV94 I'm not sure how to write the test without actually sampling. In the similar tests under The new logic of determining Maybe I'm misunderstanding what you mean. |
|
You're right @JasonTam, I was confused by the fact that the changes in constant data are checked inside the compile_forward_function but the coords are not. I guess this is the case because coords are not part of the graph while SharedVariables are. |
| @@ -1669,6 +1670,20 @@ def test_Triangular( | |||
| assert prior["target"].shape == (prior_samples, *shape) | |||
|
|
|||
|
|
|||
| def test_get_constant_coords(): | |||
| with pm.Model() as model: | |||
There was a problem hiding this comment.
Can we add a more integration-like test like?
Spit-balling, perhaps something like:
with pm.Model() as m:
m.add_coord("trial", length=3)
x = pm.Normal("x", shape="trial")
y = pm.Deterministic("y", x.mean())
# pass dims as well
idata = az.from_dict("x": [[np.pi, np.pi, np.pi]])
with m:
pp1 = pm.sample_posterior_predictive(idata, var_names=["y"]).posterior_predictive
assert pp1["y"] == np.pi
with m:
# change coord length
pp2 = pm.sample_posterior_predictive(idata, var_names=["y"]).posterior_predictive
assert pp2["y"] != np.piThere was a problem hiding this comment.
Does this look ok? d40fffe
| ).posterior_predictive | ||
| assert pp_same_len["y"] == np.pi | ||
|
|
||
| # Coord length changed -- `x` is volatile |
There was a problem hiding this comment.
I prefer to change the dim length in the pymc model and pass exactly the same idata to sample_posterior_predictive as was done in the first call.
There was a problem hiding this comment.
The way to update an existing dim is with Model.set_dim()
There was a problem hiding this comment.
Sorry, just got to this now. Implemented in 49bbf8c -- hope I understood you correctly
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
|
Thanks @JasonTam |
Description
Allow coords defined by length only (no values) to be considered
constant_coordsif the new length matches.Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7381.org.readthedocs.build/en/7381/