Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #143 +/- ##
===========================================
+ Coverage 97.75% 97.83% +0.07%
===========================================
Files 37 38 +1
Lines 2493 2582 +89
Branches 422 438 +16
===========================================
+ Hits 2437 2526 +89
Misses 32 32
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rozyczko
left a comment
There was a problem hiding this comment.
I think you need to address an unknown function issue.
| try: | ||
| self._expr = sp.sympify(expression, locals=locals_dict) | ||
| except Exception as e: |
There was a problem hiding this comment.
There is no post-parse validation that all function calls are in the allowlist.
E.g.
ExpressionComponent("foo(x)") will initialize successfully, but evaluate raises NameError when calling self._func(*args)
Is this as designed?
>>> from easydynamics.sample_model import ExpressionComponent as ec
>>> expr = ec("foo(x)")
>>> expr.evaluate(1)
File "C:\projects\easy\dynamics-lib\src\easydynamics\sample_model\components\expression_component.py", line 198, in evaluate
return self._func(*args)
^^^^^^^^^^^^^^^^^
File "<lambdifygenerated-1>", line 2, in _lambdifygenerated
NameError: name 'foo' is not definedThere was a problem hiding this comment.
How about a simple validator?
# Reject unknown functions early so invalid expressions fail at init,
# not later during numerical evaluation.
allowed_function_names = set(self._ALLOWED_FUNCS) | {
func.__name__ for func in self._ALLOWED_FUNCS.values()
}
# Walk all function-call nodes in the parsed expression (e.g. sin(x), foo(x)).
# Keep only function names that are not in our allowlist.
unknown_function_names: set[str] = set()
function_atoms = self._expr.atoms(sp.Function)
for function_atom in function_atoms:
function_name = function_atom.func.__name__
if function_name not in allowed_function_names:
unknown_function_names.add(function_name)
unknown_functions = sorted(unknown_function_names)
if unknown_functions:
raise ValueError(
f"Unsupported function(s) in expression: {', '.join(unknown_functions)}"
)This will show a much more informative
>>> e = ec('foo(x)')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "C:\projects\easy\dynamics-lib\src\easydynamics\sample_model\components\expression_component.py", line 128, in __init__
raise ValueError(
ValueError: Unsupported function(s) in expression: fooThere was a problem hiding this comment.
Very good point! I somehow did not consider this case, not even when playing around with the implementation to see if it was robust.
rozyczko
left a comment
There was a problem hiding this comment.
This now looks good.
But it would be also useful for others to have a look
So that users can easily define their own models.
Example: