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

GSYE-839: Corrected savings calculation, taking into account by inclu… #1825

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

spyrostz
Copy link
Member

…ding the savings from the revenue in addition to the savings from buying energy from community. Corrected the default grid fee reduction value to be 1, so that intracommunity trading does not include any grid fees by default.

Reason for the proposed changes

Please describe what we want to achieve and why.

Proposed changes

INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=master

…ding the savings from the revenue in addition to the savings from buying energy from community. Corrected the default grid fee reduction value to be 1, so that intracommunity trading does not include any grid fees by default.
@spyrostz spyrostz requested a review from a team January 12, 2025 14:40
Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 59.37500% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.59%. Comparing base (67e3a07) to head (caacce5).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1825      +/-   ##
==========================================
- Coverage   69.61%   69.59%   -0.03%     
==========================================
  Files         150      150              
  Lines       14227    14247      +20     
  Branches     2666     2670       +4     
==========================================
+ Hits         9904     9915      +11     
- Misses       3794     3805      +11     
+ Partials      529      527       -2     

hannesdiedrich
hannesdiedrich previously approved these changes Jan 13, 2025
Copy link
Member

@hannesdiedrich hannesdiedrich left a comment

Choose a reason for hiding this comment

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

LGTM, only left some minor comments

@@ -278,7 +278,7 @@ class AreaFees:

grid_import_fee_const: float = 0.0
grid_export_fee_const: float = 0.0
grid_fees_reduction: float = 0.0
grid_fees_reduction: float = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -539,14 +557,17 @@ class AreaEnergyBillsWithoutSurplusTrade(AreaEnergyBills):
"""Dedicated AreaEnergyBills class, specific for the non-suplus trade SCM."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Dedicated AreaEnergyBills class, specific for the non-suplus trade SCM."""
"""Dedicated AreaEnergyBills class, specific for the non-surplus trade SCM."""

Comment on lines +308 to +310
comm_bills["savings"] = savings
comm_bills["savings_from_buy_from_community"] = savings_from_buy_from_community
comm_bills["savings_from_sell_to_community"] = savings_from_sell_to_community
Copy link
Member

Choose a reason for hiding this comment

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

So you decided to export all of them, even though the "savings" already include the other two for the "normal" AreaEnergyBills ?
I guess this is in order not to break the existing workflow, right? However, I do not see the last two begin used on the gsy-web. Can you please point me to them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually thinking to save these in the DB for debugging purposes, but later on I figured out that it is probably an overkill to maintain them just for debugging. Will remove these both from here and from the gsy-framework, thanks for catching this!

@spyrostz spyrostz merged commit 1567499 into master Jan 13, 2025
2 of 4 checks passed
@spyrostz spyrostz deleted the bug/GSYE-839 branch January 13, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants