Drop formula step Averager

[christianparpart]

The Averager step was a quick hack made to provide an average soc stream from the logical meter. This was a temporary measure, until soc was available from the battery pool. That requirement is now gone, and for the reasons listed below, it can't be made a part of the public API, so it must be removed.

The Averager step has a number of issues that make it harder to integrate into our formula engine setup:

• it takes only receivers
• it can't be composed with other steps, and has to be the only step in a formula.
• it doesn't decompose to a binary operation like max/min can, i.e avg(4, 5, 6) is not the same as avg(avg(4, 5), 6), so it can't be integrated into our postfix expression evaluator.

All the above can be changed, but not without sacrificing something. But we should limit the proliferation of steps that don't fit together, and can be flaky if we try to force them to work with other operations, and where every such operation needs special treatment from the developers. Especially for steps for which we don't have a clear use-case.

[tiyash-basu-frequenz]

Sorry, I do not remember the discussion. 😞
Maybe the commit description and the release can have more context about why this is being dropped?

[christianparpart]

Sorry, I do not remember the discussion. 😞

Sorry @tiyash-basu-frequenz. I meant @sahas-subramanian-frequenz. it was right next at my desk. But it doesn't matter too much. :)

IIRC, this is dropped because it is a relic from the very early days, and not used at all, and not even exposed to the public. And the latter is the reason why I did not add a release notes item, because it's not publically facing :)

[matthias-wende-frequenz]

@sahas-subramanian-frequenz can you give more context? Is it buggy? Are there other problems related to it? IMO it's a useful operation to have.

[llucax]

as discussed with @tiyash-basu-frequenz,

It would be nice to clarify what you discussed so other can know the reasoning behind this change, it is pretty mysterious then put like this :)

I see you expanded on the reason. I will put that into the PR body, so it is more visible and will be part of the merge commit too if this gets in.

[llucax]

BTW, if that reason was not accurate (it has a IIRC), please update the body with the current/correct one. Also remember that PR bodies are used as merge commit messages, so try to write those as you would write a commit message.

[christianparpart]

I'm afraid I'd be getting the wording wrong. So without shooting myself into my foot, maybe @sahas-subramanian-frequenz knows the best :-)

[shsms]

The Averager step was a quick hack made to provide an average soc stream from the logical meter. This was a temporary measure, until soc was available from the battery pool. That requirement is now gone, and for the reasons listed below, it can't be made a part of the public API, so it must be removed.

The Averager step has a number of issues that make it harder to integrate into our formula engine setup:

• it takes only receivers
• it can't be composed with other steps, and has to be the only step in a formula.
• it doesn't decompose to a binary operation like max/min can, i.e avg(4, 5, 6) is not the same as avg(avg(4, 5), 6), so it can't be integrated into our postfix expression evaluator.

All the above can be changed, but not without sacrificing something. But we should limit the proliferation of steps that don't fit together, and can be flaky if we try to force them to work with other operations, and where every such operation needs special treatment from the developers. Especially for steps for which we don't have a clear use-case.

What we really need is a new design concept based on a tree-walking evaluator, that would allow for the composition of arbitrary operations - binary or otherwise, seamlessly, without causing additional async cycles, just from adding an implementation for the operation, without having to touch implementation internals, like we're having to do operator priority in the current expression evaluator.

[llucax]

I would say put this in the PR description:

The Averager step was a quick hack made to provide an average soc stream from the logical meter. This was a temporary measure, until soc was available from the battery pool. That requirement is now gone, and for the reasons listed below, it can't be made a part of the public API, so it must be removed.

The Averager step has a number of issues that make it harder to integrate into our formula engine setup:

• it takes only receivers
• it can't be composed with other steps, and has to be the only step in a formula.
• it doesn't decompose to a binary operation like max/min can, i.e avg(4, 5, 6) is not the same as avg(avg(4, 5), 6), so it can't be integrated into our postfix expression evaluator.

All the above can be changed, but not without sacrificing something. But we should limit the proliferation of steps that don't fit together, and can be flaky if we try to force them to work with other operations, and where every such operation needs special treatment from the developers. Especially for steps for which we don't have a clear use-case.

And maybe create a new issue with this?

What we really need is a new design concept based on a tree-walking evaluator, that would allow for the composition of arbitrary operations - binary or otherwise, seamlessly, without causing additional async cycles, just from adding an implementation for the operation, without having to touch implementation internals, like we're having to do operator priority in the current expression evaluator.

[llucax]

This still has release notes conflicts.