Skip to content

Conversation

@shsms
Copy link
Collaborator

@shsms shsms commented Oct 21, 2024

This PR adds formula generators for various microgrid metrics that traverse the graph to produce and return string formulas.

Also move the code for the module from `graph.rs` to
`graph/test_utils`.

This is to make room for a component graph builder for tests, that
will be added in the next commit.

Signed-off-by: Sahas Subramanian <[email protected]>
Copy link

@llucax llucax left a comment

Choose a reason for hiding this comment

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

I gave this a quick look and looks really nice, with all the automatic fallaback/COALESCE. As a no-rust, it feels a bit the docs are not very verbose though. Can we maybe generate the docs as GitHub pages? Or will they be automatically generated and published when uploading to crates.io?

It looks to me that maybe more in depth docs about how do the formula builders work (what kind of components are considered, which are used as fallback of which, etc.) might be helpful for users to know.

Copy link

@ela-kotulska-frequenz ela-kotulska-frequenz left a comment

Choose a reason for hiding this comment

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

I survived until commit: Support generating formulas with fallback expressions :)
Will check rest later today or tomorrow.

Comment on lines 78 to 95
if self
.graph
.successors(component_id)?
.all(|x| x.is_supported())
{
let mut exprs = vec![
Expr::components(
self.graph
.successors(component_id)?
.map(|x| x.component_id()),
)
.into_iter()
.reduce(|a, b| a + b)
.ok_or(Error::internal(
"Can't find successors of components with successors.",
))?,
Expr::component(component_id),
];
Copy link

@ela-kotulska-frequenz ela-kotulska-frequenz Oct 23, 2024

Choose a reason for hiding this comment

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

Either I don't understand this fragment or it is over complicated.
Would something like this work? It is just an idea

let successors = self.graph.successors(component_id)?;
if successors.len() == 0 || successors.all(|x| x.is_supported()) == false:
      return Ok(Some(Expr::component(component_id)));

let fallback_expr = successors.map(|node|  Expr::component(node.component_id)).reduce(|a, b| a + b))? 
exprs = match self.prefer_meters:
   true => vec![Expr::component(component_id), fallback_expr]
   false => vec![ fallback_expr, Expr::component(component_id)]
return  Ok(Some(Expr::coalesce(exprs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, it was possibly too nested. I've simplified this as suggested.

Choose a reason for hiding this comment

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

One more comment to this below.

@shsms shsms force-pushed the formulas branch 2 times, most recently from d23964c to c0c3d59 Compare October 24, 2024 10:13
@shsms
Copy link
Collaborator Author

shsms commented Oct 24, 2024

As a no-rust, it feels a bit the docs are not very verbose though. Can we maybe generate the docs as GitHub pages? Or will they be automatically generated and published when uploading to crates.io?

They will be generated from our inline doc comments and become available at docs.rs when we publish to crates.io.

It looks to me that maybe more in depth docs about how do the formula builders work (what kind of components are considered, which are used as fallback of which, etc.) might be helpful for users to know.

More documentation is necessary, especially definitions of formula components, instructions for using the formulas, and explanations for the algorithms. But I'll get to those after the formulas and algorithms have stabilized; otherwise, I'll spend all my time rewriting the docs.

Copy link

@ela-kotulska-frequenz ela-kotulska-frequenz left a comment

Choose a reason for hiding this comment

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

Checked until Add the consumer formula generator

Comment on lines 114 to 120
/// the matching components.
pub(crate) fn find_all(
&self,
from: u64,
mut pred: impl FnMut(&N) -> bool,
follow_after_match: bool,

Choose a reason for hiding this comment

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

Minor: I don't understand why it is called find_all?
Because It doesn't finds all nodes. It finds only nodes in direction outgoing. :)

Maybe find_with_pred would be more descriptive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially, it also had a direction parameter, but I realized we didn't need it. I will think of an option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a direction parameter as a filter instead.

shsms added 12 commits November 1, 2024 16:39
Fallback expressions may be used when the primary expressions are
missing values.

Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
With the new consumer power formula, it is no longer necessary to
identify meters as grid meters.  So this method can go away.

If the SDK's traversal algorithms need it, they can use their own
temporary implementation, until they can migrate to the new formulas.

Signed-off-by: Sahas Subramanian <[email protected]>
shsms added 3 commits January 6, 2025 11:30
All components now have a fallback to 0.0, so this check is no longer
necessary.

Signed-off-by: Sahas Subramanian <[email protected]>
Earlier the direction was hard coded, which made it unclear that the
method was only finding outgoing nodes.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jan 9, 2025
@shsms shsms merged commit 4ddea4b into frequenz-floss:v0.x.x Jan 9, 2025
2 checks passed
@shsms shsms deleted the formulas branch January 9, 2025 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants