-
Notifications
You must be signed in to change notification settings - Fork 13
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
new ExprTK-based Expression evaluation blocks #486
Conversation
b389099
to
270e6f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, and surprisingly small implementation footprint for such a powerful feature 👍
Could be merged as is as soon as the CI and formatting is fixed.
Since the CI anyway needs another iteration I added some minor comments. I only commented on the first Block implementation but most of them are valid to the other 2 as well. In addition I noticed that the naming of the inputs and outputs in the expression strings is a bit inconsistent between the different implementations now, i wrote up some alternative ideas to make it more consistent, but since consistency isn't the only goal here, the original might still be the best choice.
ExpressionSISO: x -> y (proposal: x -> y | x -> p | in -> out)
ExpressionDISO: x,y -> z (proposal: x1,x2 -> y | x,y -> p | in1, in2 -> out)
ExpressionBulk: inVec -> outVec (proposal: X -> Y | X -> P | inVec -> outVec)
auto [vecSize, vecIndex] = computeVectorInfo(context.base_ptr, context.end_ptr, typeSize, context.access_ptr); | ||
throw gr::exception(fmt::format("vector access '{name}[{index}]' outside of [0, {size}[ (typesize: {typesize})", // | ||
fmt::arg("name", vector_name), fmt::arg("size", vecSize), fmt::arg("index", vecIndex), fmt::arg("typesize", typeSize))); | ||
return false; // should never reach here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code due to the throw directly above.
For custom state handling logic, I see the argument of having such a "never reached" statement, but here we effectively guard against something the compiler guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless exceptions are disabled...
A<T, "a", Doc<"free parameter 'a' for use in expressions">, Visible> param_a = T(1.0); | ||
A<T, "b", Doc<"free parameter 'b' for use in expressions">, Visible> param_b = T(0.0); | ||
A<T, "c", Doc<"free parameter 'c' for use in expressions">, Visible> param_c = T(0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this so all 3 parameters have the same initial value.
PortIn<T> in; | ||
PortOut<T> out; | ||
|
||
A<std::string, "expr string", Doc<"for syntax see: https://github.com/ArashPartow/exprtk">> expr_string = "clamp(-1.0, sin(2 * pi * x) + cos(x / 2 * pi), +1.0)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use something more trivial as the default expression, maybe:
A<std::string, "expr string", Doc<"for syntax see: https://github.com/ArashPartow/exprtk">> expr_string = "clamp(-1.0, sin(2 * pi * x) + cos(x / 2 * pi), +1.0)"; | |
A<std::string, "expr string", Doc<"for syntax see: https://github.com/ArashPartow/exprtk">> expr_string = "0"; |
Just in case the initialization fails, no signal is easier to diagnose than getting some arbitrary signal. We have examples in the docstring and unittests.
exprtk::symbol_table<T> _symbol_table; | ||
exprtk::expression<T> _expression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exprtk::symbol_table<T> _symbol_table; | |
exprtk::expression<T> _expression; | |
exprtk::symbol_table<T> _symbol_table{}; | |
exprtk::expression<T> _expression{}; |
allows to use the parent's constructors.
@@ -35,7 +35,7 @@ Commonly used for testing and simulations where consistent output and finite exe | |||
Annotated<gr::Size_t, "max samples", Doc<"count>n_samples_max -> signal DONE (0: infinite)">> n_samples_max = 0U; | |||
Annotated<gr::Size_t, "count", Doc<"sample count (diagnostics only)">> count = 0U; | |||
|
|||
GR_MAKE_REFLECTABLE(ConstantSource, out, n_samples_max, count); | |||
GR_MAKE_REFLECTABLE(ConstantSource, out, default_value, n_samples_max, count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add a small followup for this: since default_value was of type T and the Sinks are instantiated with a T=UncertainValue, which is not supported as a setting type i had to change the type to value_t -> cannot set default values with errors or imaginary parts for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qa_FileIO is working again, but for non-trivial types the sources still cause some conversion errors and unexpected results.
try { | ||
expect(sched.runAndWait().has_value()); | ||
expect(false) << fmt::format("should have failed"); | ||
} catch (const gr::exception& ex) { | ||
expect(true); | ||
fmt::println("failed correctly with:\n{}\n", ex); | ||
} catch (...) { | ||
expect(false) << fmt::format("caught unknown/unexpected exception"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use ut's builtin exception matcher?
try { | |
expect(sched.runAndWait().has_value()); | |
expect(false) << fmt::format("should have failed"); | |
} catch (const gr::exception& ex) { | |
expect(true); | |
fmt::println("failed correctly with:\n{}\n", ex); | |
} catch (...) { | |
expect(false) << fmt::format("caught unknown/unexpected exception"); | |
} | |
expect(throws<gr::exception>([] { expect(sched.runAndWait().has_value()); })) |
https://github.com/boost-ext/ut/blob/master/README.md?plain=1#L1116
ba5c799
to
e4f3cf7
Compare
e4f3cf7
to
d2c1757
Compare
* ExpressionSISO: Single-Input-Single-Output * ExpressionDISO: Dual -Input-Single-Output * ExpressionBulk -> std:span-Input-to-std::span-Output. Signed-off-by: rstein <r.steinhagen@gsi.de> Signed-off-by: Ralph J. Steinhagen <r.steinhagen@gsi.de> Signed-off-by: Alexander Krimm <A.Krimm@gsi.de>
d2c1757
to
389d30b
Compare
Quality Gate failedFailed conditions |
This PR introduces a set of new expression-based blocks --
ExpressionSISO
,ExpressionDISO
, andExpressionBulk
-- which use the ExprTK library for dynamically defined mathematical operations on input samples based on string-based expressions at runtime:ExpressionSISO -- Single-Input-Single-Output (SISO):
evaluates an expressions
a*x + b
,sin(pi*x)
, or even recursive forms likey := y + 0.1*x
.ExpressionDISO -- Dual-Input-Single-Output (DISO):
Process two input streams simultaneously, e.g.,
z := a*(x+y)
orz := inrange(-1, x+y, 1) ? (x+y) : 0
.ExpressionBulk -- Bulk (Vector) Processing:
Operate on entire arrays at once, enabling expressions like:
This supports loops, indexing, and complex transformations.
Optional runtime checks can detect out-of-range vector indexing and raise exceptions.
For more examples -- especially regarding the possible syntax -- see: