Skip to content

Add expression component#143

Open
henrikjacobsenfys wants to merge 3 commits intodevelopfrom
expression-component
Open

Add expression component#143
henrikjacobsenfys wants to merge 3 commits intodevelopfrom
expression-component

Conversation

@henrikjacobsenfys
Copy link
Copy Markdown
Member

So that users can easily define their own models.
Example:

        expr = ExpressionComponent(
            "A * exp(-(x - x0)**2 / (2*sigma**2))",
            parameters={"A": 10, "x0": 0, "sigma": 1},
        )

@henrikjacobsenfys henrikjacobsenfys added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority labels Mar 29, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.83%. Comparing base (cec1976) to head (d29f314).
⚠️ Report is 1 commits behind head on develop.

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              
Flag Coverage Δ
integration 49.10% <28.08%> (-0.76%) ⬇️
unittests 97.83% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to address an unknown function issue.

Comment on lines +110 to +112
try:
self._expr = sp.sympify(expression, locals=locals_dict)
except Exception as e:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 defined

Copy link
Copy Markdown
Member

@rozyczko rozyczko Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: foo

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point! I somehow did not consider this case, not even when playing around with the implementation to see if it was robust.

Copy link
Copy Markdown
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now looks good.
But it would be also useful for others to have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[priority] medium Normal/default priority [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants