Skip to content
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

Name single phase and 3-phase metrics consistently #880

Open
daniel-zullo-frequenz opened this issue Feb 15, 2024 · 4 comments
Open

Name single phase and 3-phase metrics consistently #880

daniel-zullo-frequenz opened this issue Feb 15, 2024 · 4 comments
Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users
Milestone

Comments

@daniel-zullo-frequenz
Copy link
Contributor

daniel-zullo-frequenz commented Feb 15, 2024

What's needed?

The name for the metrics streamers are not consistent in term of single phase or 3-phase values that they stream.

microgrid.grid().power.new_receiver() # single phase values

microgrid.grid().current.new_receiver() # 3-phase tuple

microgrid.voltage().new_receiver() # 3-phase tuple
 
microgrid.frequency.new_receiver() # single phase values

The SDK will be adding more 3-phase metrics in the near future so it'll be helpful to distinguish the type of values provided by each metric in the name.

Proposed solution

A first idea is to append a suffix to the 3-phase metrics (e.g. _per_phase). This still needs to be discussed and agreed on.

microgrid.grid().power.new_receiver() # single phase values

microgrid.grid().current_per_phase.new_receiver() # 3-phase tuple

microgrid.voltage_per_phase().new_receiver() # 3-phase tuple
 
microgrid.frequency.new_receiver() # single phase values

Use cases

No response

Alternatives and workarounds

No response

Additional context

No response

@daniel-zullo-frequenz daniel-zullo-frequenz added part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed type:enhancement New feature or enhancement visitble to users priority:low This should be addressed only if there is nothing else on the table part:data-pipeline Affects the data pipeline and removed part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed labels Feb 15, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz added this to the v1.0.0-rc6 milestone Feb 15, 2024
@llucax
Copy link
Contributor

llucax commented Feb 15, 2024

What do power, current and frequency returns in the case of the single value? Because if in reality these are all 3 phases always and for the single value we are doing some sort of projection or averaging of these 3 values, maybe the single value should be named differently, like power_average or something.

I also remember @matthias-wende-frequenz saying that we don't really always have 3 phases, what happens in that case, is power_3_phase raising an exception? It will create the receiver but not send any values? It will create the receiver and send None as Sample3Phase? It will create the receiver and send a Sample3Phase with a value only in one phase and the rest 0 or None?

@shsms
Copy link
Contributor

shsms commented Feb 16, 2024

The power formulas produce 3-phase power, but summed up, as a single value.

Maybe the right notation is power_3_phase (for a single value) and power_per_phase (for per-phase values).

I would rather leave power as it is, and just rename current to current_per_phase, etc.

@llucax
Copy link
Contributor

llucax commented Feb 19, 2024

If we won´t always have 3 phases, not sure if the "sum of all available phases" should be called power_3_phase, unless we consider the case with less phases to always be 3 phases, but some phases have no electricity flowing through them.

I agree in leaving the most commonly used value as just power and current, specially if it is the most widely meaning for the term in the presence of multiple phases.

We also have another layer of confusion, which is true, apparent and reactive power (and does that go for current and voltage too?), so in that sense I'm not sure if we should at least that encoded into the metric/attribute name.

@shsms
Copy link
Contributor

shsms commented Feb 19, 2024

The field we call power is the active power, and that's the most common usage. This is measured in Watts. This is also the the power that would mostly be set for business use cases. So I think we should leave that as it is.

Reactive power needs to be controlled to maintain the power factor, to avoid fines, etc, and so is not supposed to be dealt with in the most high level applications. It also has a different unit (VAR).

Once we start to stream reactive power from the sdk or allow controlling reactive power, we can call those reactive_power or set_reactive_power.

@llucax llucax modified the milestones: v1.0.0-rc6, v1.0.0-rc7 Mar 26, 2024
@llucax llucax added scope:breaking-change Breaking change, users will need to update their code priority:high Address this as soon as possible and removed priority:low This should be addressed only if there is nothing else on the table labels Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users
Projects
Status: To do
Development

No branches or pull requests

3 participants