-
Notifications
You must be signed in to change notification settings - Fork 4
Streamlined SCM Variable Access Methods & Mapped Out Counterfactual Queries #3
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
base: main
Are you sure you want to change the base?
Conversation
|
It may also be good to add code examples (effectively, tests) to a separate file, perhaps a chapter5.ipynb notebook. |
scm.py
Outdated
|
|
||
| assert all(k in self.v for k in x), x | ||
|
|
||
| assert all(isinstance(val, int) or (val == self._counterfactuals[val].syn[k]) |
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.
Can we make this more general, to allow for arbitrary interventional expressions, which may use counterfactual symbols?
m = SymbolicSCM(f=dict(z=..., x=..., w=..., y=...), pu={...})
z_1, x_1, w_1, y_1 = m.do({x: 1}).v
# all of these could be valid
m.do({x: 1})
m.do({x: z ^ 1})
m.do({w: x_1})
m.do({w: x_1 | z})
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.
How should I approach z ^ 1? It gives me an error about performing operations on sp.symbols vs int. I would normally implement a wrapper, but it happens before it is passed as a param.
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.
Oops. Let me correct that second do line to one or both of:
m.do({x: ~z})
m.do({w: x ^ z})
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.
All expressions are now ingestible, but I'm not sure if I can guarantee meaningful results
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.
Pull Request Overview
This PR introduces a streamlined variable access system for Structural Causal Models (SCMs) and implements foundational components for counterfactual queries. The changes enhance the user interface for accessing SCM variables and establish infrastructure for handling counterfactual worlds.
- Implements enhanced variable access patterns allowing index, value, and attribute-based retrieval
- Adds core symbolic classes for probabilistic expressions and counterfactual variables
- Establishes framework for multi-world networks and counterfactual graph operations
Reviewed Changes
Copilot reviewed 36 out of 142 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/citk/sympy_classes/variable.py | Defines Variable class with intervention tracking for counterfactual analysis |
| src/citk/return_classes/symbol_container.py | Implements SymbolContainer with multiple access patterns (index, attribute, value) |
| src/citk/scm/init.py | Updates SymbolicSCM with streamlined variable access and counterfactual support |
| src/citk/causal_graph/init.py | Adds CausalGraph class with d-separation and adjustment methods |
| src/citk/sympy_classes/pr.py | Implements Pr class for probabilistic expressions with LaTeX rendering |
Comments suppressed due to low confidence (2)
src/citk/causal_graph/accessors.py:1
- The method accesses
self._ctf_graphsbut this attribute is not defined in the abstract base class. This will cause an AttributeError when called on classes that don't define this attribute.
from abc import ABC, abstractmethod
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
Copilot
AI
Sep 14, 2025
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.
Missing import statement for sympy. The class inherits from sp.Expr but sympy is not imported as sp.
| import sympy as sp |
| A list of Variable instances. | ||
| """ | ||
|
|
||
| tup = [Variable(name.strip()) for name in names.split(" ")] |
Copilot
AI
Sep 14, 2025
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.
The function splits on spaces but the docstring indicates comma-separated names. Either change the split to use commas or update the docstring to reflect space separation.
| tup = [Variable(name.strip()) for name in names.split(" ")] | |
| tup = [Variable(name.strip()) for name in names.split(",")] |
|
|
||
| import itertools | ||
| import math | ||
|
|
||
| from typing import Dict, List, Optional, Union | ||
|
|
||
| import pandas as pd | ||
| import sympy as sp | ||
| from IPython.display import Latex | ||
|
|
||
| from src.sympy_classes.pr import Pr | ||
| from src.return_classes import SymbolContainer | ||
|
|
||
| from src import CausalGraph | ||
|
|
||
| from src.sympy_classes.summation import Summation | ||
| from src.sympy_classes.variable import Variable | ||
|
|
||
|
|
||
|
|
Copilot
AI
Sep 14, 2025
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.
[nitpick] Import statements should be grouped and sorted. Group standard library imports first, then third-party imports, then local imports, with blank lines between groups.
| import itertools | |
| import math | |
| from typing import Dict, List, Optional, Union | |
| import pandas as pd | |
| import sympy as sp | |
| from IPython.display import Latex | |
| from src.sympy_classes.pr import Pr | |
| from src.return_classes import SymbolContainer | |
| from src import CausalGraph | |
| from src.sympy_classes.summation import Summation | |
| from src.sympy_classes.variable import Variable | |
| import itertools | |
| import math | |
| from typing import Dict, List, Optional, Union | |
| import pandas as pd | |
| import sympy as sp | |
| from IPython.display import Latex | |
| from src import CausalGraph | |
| from src.return_classes import SymbolContainer | |
| from src.sympy_classes.pr import Pr | |
| from src.sympy_classes.summation import Summation | |
| from src.sympy_classes.variable import Variable |
| v[k] = self.f[k].subs(v) | ||
| return v | ||
|
|
||
| def _evaluate_symbolic_expression(self, expression: sp.Expr, u:Dict[Variable,int]) -> int: |
Copilot
AI
Sep 14, 2025
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.
Returning hash(output) as an integer is misleading since the method name suggests it evaluates a symbolic expression. Consider returning the actual evaluated value or renaming the method to reflect its hash-based behavior.
| return pt.query(query).probability.sum() | ||
|
|
||
| def do(self, x: Dict[sp.Symbol, int]) -> SymbolicSCM: | ||
| def do(self, x: Dict[Variable, Union[sp.Expr, int, Variable]]) -> SymbolicSCM: |
Copilot
AI
Sep 14, 2025
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.
The method calls k.update_interventions(x) for every variable in self.v, but interventions in x only apply to specific variables. Consider filtering to only update variables that are actually being intervened upon.
Proposed location for recursive querying of counterfactual worlds.
Modified do() method to check for valid counterfactual operations (same domain).
Streamlined variable access methods by allowing for index or value retrieval; e.g., scm.v.x, scm.v['x'], scm.v[0], scm.v