Make do interventions shared variables by default #7596
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7596 +/- ##
==========================================
- Coverage 92.84% 92.83% -0.01%
==========================================
Files 106 106
Lines 17719 17723 +4
==========================================
+ Hits 16451 16454 +3
- Misses 1268 1269 +1
|
| if isinstance(var, str): | ||
| var = model[var] | ||
| try: | ||
| do_mapping[var] = var.type.filter_variable(obs) | ||
| intervention = var.type.filter_variable(intervention) | ||
| if make_interventions_shared and isinstance(intervention, Constant): |
There was a problem hiding this comment.
To me make_interventions_shared is easier than the past version, but I think that it should be more stringent. I think that if make_interventions_shared is True, it should wrap the intervention with a shared regardless of the type of data provided (be it a shared, a constant or a numpy array). Furthermore, if make_interventions_shared is False I think that the interventions should be set to Contants.
There was a problem hiding this comment.
I'll have to review the code, but the thing is intervetions may not be constants, they could be pytensor graphs depending on other variables or input variables. In which case they can't be made Shared nor Constant. This check for isinstance(intervention, Constant) was just handling that.
Basically the same as calling as_tensor and checking if it comes back as a Constant or not. If not it's a more complex graph.
There was a problem hiding this comment.
Right! Ok, so then this is fine with me. But could you also handle the make_interventions_shared=False situation similarly? Something that checks if intervention is a SharedVariable instance and converts it to a Constant?
There was a problem hiding this comment.
Do we need that? Something being shared is never a problem. If the user wanted them constant he could do it themselves?
This behavior should be more intuitive, since do interventions create
Datacontainers, and users expect those to be mutable by default. This also alleviates some issues related tosample_posterior_predictivethat currently does not look for sources of mutability on constants:Closes #6977 (I tested the MWE locally)
Related to #7069
We could consider something similar for
pm.observebut that doesn't create data containers at all right now, so it's a tad less relevant and would require more changes.📚 Documentation preview 📚: https://pymc--7596.org.readthedocs.build/en/7596/