-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Allow do interventions to reference intervened variable #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know the complete details of the do
-operator implementation but his change makes sense by reading the code and test :)
Thanks for reviewing @juanitorduz @twiecki wanna give a thumbs up/down? |
Big 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Were you getting duplicate names errors before? I wouldn't have expected that though. Did the error come at the point where the new model would be recompiled from the FunctionGraph
?
The error arises when we convert fgraph to a Model and try to register multiple variables (such as an RV and a Deterministic) with the same name. This happens here because we borrow the name of the variable being intervened for the deterministic that represents the intervention. Does that make sense? |
9a8aa7b
to
8c32d44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8c32d44
to
14d4f2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a check of what new_m = do(m, {x: x + 100})
does. This might help inform extra info for the docstring for do
potentially.
My intuition of what this would do, under the principle of least surprise, would be:
- Replace the RV
x
with a newRV
but which has the +100 modifier. - Cut the incoming edges to
x
because that's a core part of whatdo
does.
Any particular ideas about possible use-cases?
Is this just constrained to some kind of deterministic modification? What if someone comes along and does new_m = do(m, {x: x + z})
where z
is another RV. If that is possible/permissible, then might be worth adding a test.
No that's not what happens. It keeps The PyMC I don't know about use cases but seems like a more powerful mechanism. |
That's possible, and under the hood is exactly the same as the test that was introduced. There are already tests for the What's new in this PR is not that the interventions can contain random variables, but that they can reference the original variable that is being intervened. |
So I think this might throw people, at least initially. The do-operator (from Pearl) is pretty well-defined, and it seems that this goes beyond that to do more generic graph surgery. I don't think I have a strong philosophical objection, but cutting incoming edges to the target node is a pretty major component of the do operator. So I guess we either need to make sure the docstrings are really clear about that, or have a different operator name for this particular manipulation of the graph. |
Same point as above. The PyMC do operator is going beyond Pearl's do operator concept. Either that can be seen as more powerful, or it could be seen as confusing. |
We can call the current one "replace" and the "do" would call replace but only allow constants/shared variables? CC @lucianopaz |
Like dispatching? That would still involve the user calling do and not deleting incoming edges? How about the user calling this new function when they want to do that. And if they try to achieve it with do they get an informative error message to use the new function instead? Could be worth jumping on a quick call to talk this through perhaps? |
I mean we rename the current function Either way I would do that in a separate PR from this one. WDYT? |
I just did a random quick search for "Pearl stochastic do interventions" and this showed up first: https://ojs.aaai.org/index.php/AAAI/article/view/6567 (pdf can be found on Google scholar) I don't know anything about the subject but if this sort of stuff is interesting/valid then the current "supercharged" I imagine stuff like |
That paper is pretty neat. Don't have the luxury to go through it in detail, but I guess it satisfies me that it's kosher to have a single do-operator which implements different kinds of interventions. The general concept of a stochastic do-operator isn't 100% novel to me, but I did perceive it as a categorically different thing. I'm more happy now that it's not. So I'm happy to withdraw any objection of this happening under the do-operator. If you wanted to have different functions that are called behind the scenes (perhaps that improves testability?) then feel free. But from an API/user perspective I'm good with this. We obviously just need to clearly document with examples in |
Though they do talk about |
Thanks @drbenvincent. I am curious if we can write-up a compelling example that showcases these forms of interventions. |
14d4f2b
to
98e13c9
Compare
This is now possible:
It was already fine to do this kind of replacement with
Deterministics
(or other RVs) as is the following: