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

Remove the consumption and production power formulas #697

Conversation

matthias-wende-frequenz
Copy link
Contributor

@matthias-wende-frequenz matthias-wende-frequenz commented Sep 25, 2023

Users must understand passive sign conventions to utilize these formulas effectively, and therefore they do not provide significant benefits.

@matthias-wende-frequenz matthias-wende-frequenz requested a review from a team as a code owner September 25, 2023 15:08
@github-actions github-actions bot added the part:data-pipeline Affects the data pipeline label Sep 25, 2023
@cwasicki
Copy link
Collaborator

Users must understand passive sign conventions to utilize these formulas effectively, and therefore they do not provide significant benefits.

What do you mean? These formulas were introduced exactly for the reason that passive sign convention can be confusing.

@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Sep 26, 2023
@github-actions github-actions bot added the part:docs Affects the documentation label Sep 26, 2023
@shsms
Copy link
Contributor

shsms commented Sep 26, 2023

I will leave it for @cwasicki to approve, because he has open questions, but if we are doing this, we should also remove the corresponding formulas from the battery pool and the ev charger pool.

@matthias-wende-frequenz
Copy link
Contributor Author

Possible approaches for keeping the functionality alive

We discussed that these formulas are useful but they might lead to subtle bugs when confusing the slightly different functions.
We can provide this by adding a production and consumption operators as suggest in the following.

1

chp_power("production")
chp_power("abs_production")
chp_power().production() or production_op(chp_power())

2

chp_power().production() and chp_power().consumption()

@llucax
Copy link
Contributor

llucax commented Sep 27, 2023

Option 2 wins! BTW, Option 1.3 is Option 2, right?

@cwasicki
Copy link
Collaborator

Option 1.3 is Option 2, right?

Yes, but option 1.3a only ;-)

@matthias-wende-frequenz matthias-wende-frequenz marked this pull request as draft October 2, 2023 09:01
@matthias-wende-frequenz
Copy link
Contributor Author

Back to draft as it needs the operators to get implemented.

@llucax llucax changed the base branch from v0.x.x to v1.x.x October 11, 2023 07:21
@llucax llucax modified the milestones: v1.0.0-rc2, v1.0.0-rc3 Oct 13, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2023
In order to simplify usability but don't loose functionality we decided
to remove the consumption and production power formulas, i.e. the
formulas that are having a production or consumption attached to their
name, and replace them with operators that can be called on a formula
engine.

The consumption operator returns either the identity if the power value
is
positive or 0 and the production operator returns either the identity if
the power value is
negative or 0.

As an example we can now create the following formula:

```python
grid_consumption = logical_meter.grid_power().consumption().build()
```

This would return the consumption part of the grid_power stream.

Removing the old consumption and production power formulas will happen
in the follow up PR
#697.
@matthias-wende-frequenz matthias-wende-frequenz force-pushed the prod_cons branch 2 times, most recently from 2a09dd6 to af9152b Compare October 30, 2023 16:22
@matthias-wende-frequenz matthias-wende-frequenz marked this pull request as ready for review October 31, 2023 08:57
@matthias-wende-frequenz
Copy link
Contributor Author

unblocked after #746 has been merged as a replacement

llucax
llucax previously approved these changes Oct 31, 2023
Copy link
Contributor

@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.

Yeeeiii! Removing code and special cases! 🎉

Comment on lines 437 to 387
grid_consumption_recv = logical_meter.grid_consumption_power.new_receiver()
grid_consumption_recv = logical_meter.grid_power.new_receiver()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you are purposely not using consumption (with the new operator) anymore here because this is already tested in the new operator tests, right?

These formulas are replaced by a combination of the corresponding
passive sign convention formulas and the recently added consumption
and production operators.

Signed-off-by: Matthias Wende <matthias.wende@frequenz.com>
Signed-off-by: Matthias Wende <matthias.wende@frequenz.com>
Signed-off-by: Matthias Wende <matthias.wende@frequenz.com>
The formula Type has been used to distinguish between
consumption, production or passive sign convention formulas.
With the recent changes the sdk only provides passive sign
convention formulas.

Signed-off-by: Matthias Wende <matthias.wende@frequenz.com>
Signed-off-by: Matthias Wende <matthias.wende@frequenz.com>
@matthias-wende-frequenz matthias-wende-frequenz added this pull request to the merge queue Nov 1, 2023
Merged via the queue into frequenz-floss:v1.x.x with commit 89ca11e Nov 1, 2023
@matthias-wende-frequenz matthias-wende-frequenz deleted the prod_cons branch November 1, 2023 10:33
@Marenz
Copy link
Contributor

Marenz commented Jan 11, 2024

Some of the commits here are a great example of unhelpful commit title/messages.
Just reading the commit I had no idea why it was doing what it was doing "Remove xyz"..great.. but why?!
I had to find this PR first to get an idea of why it was happening.

Please, write the why, not the what. It was very very obvious that it was removing something... no need to mention that even imo.

@matthias-wende-frequenz
Copy link
Contributor Author

Just reading the commit I had no idea why it was doing what it was doing "Remove xyz"..great.. but why?!

The reason is written up in the first of the commit chain.

These formulas are replaced by a combination of the corresponding
passive sign convention formulas and the recently added consumption
and production operators.

Next time I'll add a note to each of the involved commits ;).

@Marenz
Copy link
Contributor

Marenz commented Jan 29, 2024

The reason is written up in the first of the commit chain.

right, thus I said "some" ;) but I still had to do more investigation to be sure.

Next time I'll add a note to each of the involved commits ;).

<3

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 part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
Development

Successfully merging this pull request may close these issues.

5 participants