Closed tlambert03 closed 1 year ago
Merging #31 (feec08d) into main (2495bb7) will decrease coverage by
0.20%
. The diff coverage is90.56%
.
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 92.96% 92.77% -0.20%
==========================================
Files 29 30 +1
Lines 725 747 +22
==========================================
+ Hits 674 693 +19
- Misses 51 54 +3
Impacted Files | Coverage Δ | |
---|---|---|
motile/_types.py | 0.00% <0.00%> (ø) |
|
motile/constraints/expression.py | 92.50% <92.50%> (ø) |
|
motile/constraints/__init__.py | 100.00% <100.00%> (ø) |
|
motile/constraints/pin.py | 100.00% <100.00%> (ø) |
|
motile/plot.py | 93.87% <100.00%> (ø) |
I like it, but it is also more complex than Pin
. I'd say we keep Pin
around (it can use the expressions internally) for simplicity.
Pin
also had three possible outcomes (needs to be pinned to 1
, to 0
, or not pinned if the attribute is not present). Does ExpressionConstraint
work the same way? If so, when does it decide that the constraint does not apply? If any of the referenced attributes does not exist?
I'd say we keep Pin around (it can use the expressions internally) for simplicity.
yeah Pin
is still here. It's a subclass, it looks like this now:
class Pin(ExpressionConstraint):
def __init__(self, attribute: str) -> None:
super().__init__(f"{attribute} == True", eval_nodes=True, eval_edges=True)
Pin also had three possible outcomes (needs to be pinned to 1, to 0, or not pinned if the attribute is not present). Does ExpressionConstraint work the same way?
Yes, the meat of it is here (will add more comments inline)
with contextlib.suppress(NameError):
# here we evaluate the expression with the node/edge dict as the namespace
# if a key (like the "attribute" in Pin) is missing, it will raise a NameError and we just move on
# if it is present, the expression will evaluate to a boolean. If True, we select, else we exclude
if eval(self.expression, None, node_or_edge):
select.set_coefficient(indicators[id_], 1) # type: ignore
else:
exclude.set_coefficient(indicators[id_], 1) # type: ignore
here's an example of how that works with eval
:
In [1]: eval('x == 1', {'x': 1})
Out[1]: True
In [2]: eval('x == 1', {'x': 3})
Out[2]: False
In [3]: eval('x == 1', {'y': 3})
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
Cell In[3], line 1
----> 1 eval('x == 1', {'y': 3})
File <string>:1
NameError: name 'x' is not defined
extended the class docstring and added more comments inline
Great, this looks good now. Thanks!
Hey @funkey, curious how you feel about this constraint.
It generalizes the
Pin
constraint to support any expression evaluated against the namespace of the node dictfor example, if the nodes in a graph are:
Then the following constraint will select node 0:
this uses
eval
, and therefore introduces a slight safety risk. This could be made more safe by adding a small function to check that the expression string only uses simple comparison operators (i.e. no function calls or attribute access, etc...)