Skip to content
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

Add generic expression constraint (generalization of Pin) #31

Merged
merged 9 commits into from
May 12, 2023

Conversation

tlambert03
Copy link
Member

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 dict

for example, if the nodes in a graph are:

        [
            {"id": 0, "t": 0, "color": "red", "score": 1.0},
            {"id": 1, "t": 0, "color": "green", "score": 1.0},
            {"id": 2, "t": 1, "color": "blue", "score": 1.0},
        ]

Then the following constraint will select node 0:

>>> expr = "t == 0 and color != 'green'"
>>> solver.add_constraints(ExpressionConstraint(expr))

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...)

@tlambert03 tlambert03 changed the title add expression constraint Add generic expression constraint (generalization of Pin) Mar 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #31 (feec08d) into main (2495bb7) will decrease coverage by 0.20%.
The diff coverage is 90.56%.

❗ 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%> (ø)

@funkey
Copy link
Member

funkey commented May 11, 2023

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?

@tlambert03
Copy link
Member Author

tlambert03 commented May 11, 2023

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

@tlambert03
Copy link
Member Author

extended the class docstring and added more comments inline

@tlambert03 tlambert03 merged commit 624d903 into funkelab:main May 12, 2023
10 checks passed
@funkey
Copy link
Member

funkey commented May 12, 2023

Great, this looks good now. Thanks!

@tlambert03 tlambert03 deleted the expression-constraint branch May 12, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants