Support non-default initvals in model_to_fgraph#8314
Conversation
That sentence is inconsistent if you go to t+1, model_free_rv are just syntatic sugar, free rv are actually a concept at the model level ;) The whole point of the model_fgraph is to add the meta info to reconstruct a model from the graph alone. Nit aside, I resisted adding initvals because it's one more complexity level. They can actually be symbolic variables (I don't think we restricted this), like intival = another_rv.sum(), so hiding them will break the graph on reconstruction. And even if they were only constants, it's not clear how they should propagate across model transforms. Hiding them won't help, the model transforms still have to reason about it if we truly want initvals at the model level. My ambition was to remove the vestigial signs of initval from the model altogether, and eventually make them pm.sample argument only. |
| def test_do_rewires_variable_initval_through_intervened_data(): | ||
| with pm.Model() as m: | ||
| mu_data = pm.Data("mu_data", np.array([1.0, 2.0, 3.0])) | ||
| pm.Normal("a", shape=(3,), initval=mu_data + 10) |
There was a problem hiding this comment.
what about an intervened data?
I'm not against this from first principles, but I will say that initvals on RVs "feels" like the right place they should live. The fact that it isn't is a detail about how PyMC is engineered. This is doubly true in the case of transforms (e.g. sumtozero, a common case for wanting initvals in the first place) |
|
sumtozero/ordered are cases begging for better primitives like ZeroSumNormal and #7603 Allowing initvals at the fgraph model is consenting to support them, and I'm not sure you followed the implications of this, vs telling users they have to handle initvals themselves if they want to. E.g., rountrip expectations. model -> observe() -> unobserved(), or model -> marginalize() -> unmarginalize(). We will create expectation of initvals rountrip. Similar to other operations like you already staterted testing with pm.do, and which I don't think is correct besides dependency on root / unalterede variables. The only safe initvals are the default. Otherwise I think you need to commit to the "full blast radius", a bit like we had to with the transforms. I would also have preferred not to with those, but the pragmatic sense won me over there. At least those (besides Interval) don't permit graph dependencies. Initval is a similar question, I just considered it more acceptable to drop. A trade-off may be to drop symbolic initval support, only constants or default allowed. Takes part of the fear away. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8314 +/- ##
=======================================
Coverage 91.73% 91.73%
=======================================
Files 125 125
Lines 20471 20479 +8
=======================================
+ Hits 18779 18787 +8
Misses 1692 1692
🚀 New features to boost your workflow:
|
|
Are we having a philosophical difference? My main use-case is freeze_dims_and_data, where the structure of the model is preserved 1:1. The tests i added for graph mutations just show that initvals come along for the ride, naively. I don't think we should have to do anything heroic, and users shouldn't have that expectation. If you put an initval on an RV then the RV changes meaning, tough cookies. |
Then forward the initvals only on those helpers, and leave the fgraph_from_model and cousin blissfully unaware? |
|
thinking-emoji |
2 participants
fgraph_to_modelcurrently rejects any model whose free RVs carray a non-default initval. This ends up blocking stuff likefreeze_dims_and_data,do,observed, etc., which is a bummer. This PR gathers and forwards the initvals through the model -> fgraph -> model chain, the same way_dim_lengthsalready is.I also tried a formulation where the initvals lived on the
ModelFreeRVOps, but the blast radius was too large. It's also the wrong abstraction, becauseinitvalon RVs is just syntatic sugar -- initvals are actually a concept at the model level.I elected not to clone the initvals because we don't clone anything else.