Conversation
dstrain115
commented
Oct 25, 2022
dstrain115
commented
- Sympy will soon error out if an expression is not parsable by sympy. It used to just ignore this.
- We have one test that exercises this (resolver_test.py:247). Modify code to handle this and print out a warning.
- This is breaking some internal google code and preventing sympy upgrade.
- Sympy will soon error out if an expression is not parsable by sympy. It used to just ignore this. - We have one test that exercises this (resolver_test.py:247). Modify code to handle this and print out a warning. - This is breaking some internal google code and preventing sympy upgrade.
| v = value.subs(self.param_dict, simultaneous=True) | ||
| try: | ||
| v = value.subs(self.param_dict, simultaneous=True) | ||
| except sympy.SympifyError: |
There was a problem hiding this comment.
What do you mean by "not parsable"? An actual syntax error? If so, then this is probably a user mistake and it's better to let the exception propagate out of here. Then the PR could catch it in resolver_test.py rather than in resolver.py. WDYT?
There was a problem hiding this comment.
The actual sympy bug is here:
sympy/sympy#23887
Their definition is "sympifiable" whatever that means. I think that it is either a sympy object or a basic python type, possibly a few other cases. I think it is talking about objects foreign to the sympy library. In our case, that is the symbol 'NotImplemented'.
I am hesitant to change the existing behavior. The test currently asserts that resolving {'b': NotImplemented} would just leave sympy.Symbol('b') alone. I am loathe to change this behavior right now, since there is a lot of special handling around NotImplemented.
There was a problem hiding this comment.
I agree we should preserve existing behavior.
I have two conerns:
- the fact that if one of the values is unsympifiable then none of the substitutions are made,
- the fact that we're failing to communicate a potential mistake to the user as soon as we discover it (we could be passing an unsympifiable object as a result of a simple typo in the initialization of the ParamDict).
Is NotImplemented the only offender? If so, could we just filter them out by saying something like
v = value.subs({k: w for k, w in self.param_dict.values() if w is not NotImplemented}, simultaneous=True)?
There was a problem hiding this comment.
I changed this to raise a ValueError instead, explaining what the problem substitution is.
We cannot do your suggestion, because the test case is a function that defines resolved_value as NotImplemented. This is indistinguishable from something like Sympy.Symbol('b')+Sympy.Symbol('c') which also has an unimplemented resolved_value.
This does change the behavior of the function slightly, but, as you pointed out, the previous PR would also change the value as some additional variables would not be substituted.
I think this is the best solution we can do, since this preserves the behavior 'the variable could not be substituted' but makes it explicit as a ValueError rather than silently refusing to substitute it. It's also quite a buried case, because it involves the user specifying something that was unresolvable in the first place.
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
LGTM with one optional comment: I think it's better to just let the sympy.SympifyError exception propagate rather than turning it into a ValueError. The former allows for more fine-grained handling in the client code (and also makes it obvious to a human that this is coming out of sympy which aids debugging).
| with pytest.raises(ValueError, match='Could not resolve parameter b'): | ||
| _ = r.value_of(b) |
There was a problem hiding this comment.
I think we should move this to a separate test function marked with a strict xfail, for example,
@pytest.mark.xfail(reason='this test requires sympy 1.12', strict=True)
def test_custom_value_not_implemented():
...When the new sympy is released, the strict xfail will produce a CI failure
after which we can remove the xfail mark.
There was a problem hiding this comment.
Good idea done.
|
Changed to not rethrow sympify error. |
| Raises: | ||
| RecursionError: If the ParamResolver detects a loop in recursive | ||
| resolution. | ||
| ValueError: If the resulting value cannot be interpreted. |
There was a problem hiding this comment.
Update to sympy.SympifyError?
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with Adam's comment
4 participants
