GSYE-645: Add loss calculation to storage state #1799

hannesdiedrich · 2024-09-26T05:44:32Z

Reason for the proposed changes

Please describe what we want to achieve and why.

Proposed changes

INTEGRATION_TESTS_BRANCH=feature/GSYE-645
GSY_FRAMEWORK_BRANCH=master

hannesdiedrich · 2024-09-26T06:39:25Z

src/gsy_e/models/strategy/__init__.py

@@ -1,3 +1,4 @@
+# pylint: disable=too-many-lines


This module has only be changed by Black because I fixed one method interface. You can ignore reviewing this. Thanks

codecov · 2024-09-26T06:51:21Z

Codecov Report

Attention: Patch coverage is 65.75342% with 50 lines in your changes missing coverage. Please review.

Project coverage is 69.55%. Comparing base (f442047) to head (66e2587).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1799      +/-   ##
==========================================
+ Coverage   69.52%   69.55%   +0.03%     
==========================================
  Files         148      148              
  Lines       14029    14045      +16     
  Branches     2619     2618       -1     
==========================================
+ Hits         9753     9769      +16     
- Misses       3758     3759       +1     
+ Partials      518      517       -1

BigTava

Well, I am bit skeptic that charging and discharging losses are fixed values and not percentage over the charging and discharging power. But the logic seems good ! Left minor comments but looks good

BigTava · 2024-09-26T08:23:47Z

src/gsy_e/models/strategy/state/storage_state.py

        """Register energy from a bid trade event."""
        assert energy >= -FLOATING_POINT_TOLERANCE
        self.pledged_buy_kWh[time_slot] += energy
        self.offered_buy_kWh[time_slot] -= energy
        self._track_energy_bought_type(energy, energy_origin)
        self._clamp_energy_to_buy_kWh([time_slot])
+        # if self.charging_loss_per_kWh > 0:


Maybe we can remove this

Of course, let me remove

BigTava · 2024-09-26T08:24:12Z

src/gsy_e/models/strategy/state/storage_state.py

@@ -411,6 +484,8 @@ def get_available_energy_to_buy_kWh(self, time_slot: DateTime) -> float:

        self._clamp_energy_to_buy_kWh([time_slot])
        energy_kWh = self.energy_to_buy_dict[time_slot]
+        # if self.charging_loss_per_kWh >0 and energy_kWh:


And removing this as well

hannesdiedrich · 2024-09-26T11:11:22Z

Well, I am bit skeptic that charging and discharging losses are fixed values and not percentage over the charging and discharging power.

They are indeed implemented as percentages (have a look here: https://github.com/gridsingularity/gsy-e/pull/1799/files#diff-4581ce8152beebe7aa76014b4a741adac802ef0fcd8ac7a578d7ec4aabf39dfbR362-R363)
But you are right, the names should reflect that. Will fix.

spyrostz · 2024-10-01T10:59:29Z

src/gsy_e/models/strategy/state/storage_state.py

+
+    charging_loss_percent: float = 0.0
+    discharging_loss_percent: float = 0.0
+    self_discharge_per_day_kWh: float = 0.0


All good, however the unit is striking as a bit odd here. Did you find it in any spec sheet of a battery? Reason I am asking is because I have mostly seen this unit being the percentage of the total capacity of the battery, e.g. https://www.batterypowertips.com/why-self-discharge-is-important-in-batteries/ or more specifically here https://www.batterypowertips.com/wp-content/uploads/2023/02/Why-is-self-discharge-important-in-batteries-Figure-1.jpeg .

Oh yes, it is also a percentage value. I also found the links that you shared. Let me rename. Also, I need to fix the calculation then. I don't know why I came up with the current implementation.

spyrostz · 2024-10-01T11:01:25Z

src/gsy_e/models/strategy/state/storage_state.py

+        losses: Optional[StorageLosses] = StorageLosses(),
+    ):


Minor, but since it is Optional I would prefer None as default value:

Suggested change

losses: Optional[StorageLosses] = StorageLosses(),

):

losses: Optional[StorageLosses] = None,

):

if not losses:

losses = StorageLosses()

spyrostz · 2024-10-01T11:04:10Z

src/gsy_e/models/strategy/state/storage_state.py

+        self_discharging_kWh = (
+            self.losses.self_discharge_per_day_kWh * GlobalConfig.slot_length.total_days()
+        )
+        total_loss_kWh = charging_loss_kWh + discharging_loss_kWh + self_discharging_kWh


Admittedly an overkill, but do you think that it makes sense to also keep the total_loss_kWh in the results for debugging purposes? On second thought, better to omit it and add it if necessary in the future.

I think it is not an overkill and indeed interesting to know if you run storages with losses. Let me add it to the storage CSV file.

I did add it here: 3c11349
However, there is a corner case for that last market slot. We only calculate the losses for the past market slot in the market_cycle event method. But after the last market slot this is not triggered, hence the losses are not calculated. I gave it some time to fix, but could not find a solution for this. As you named this an overkill, I left it like this now. Let us deal with the last market slot once we need it. OK?

Agreed and thanks!

spyrostz · 2024-10-01T11:07:36Z

src/gsy_e/models/strategy/storage.py

+        initial_selling_rate: Union[float, dict] = StorageSettings.SELLING_RATE_RANGE.initial,
+        final_selling_rate: Union[float, dict] = StorageSettings.SELLING_RATE_RANGE.final,
+        initial_buying_rate: Union[float, dict] = StorageSettings.BUYING_RATE_RANGE.initial,
+        final_buying_rate: Union[float, dict] = StorageSettings.BUYING_RATE_RANGE.final,
+        fit_to_limit=True,
+        energy_rate_increase_per_update=None,


Definitely not for now, but we need to migrate these constructor arguments to OrderUpdaterParameters at some point. I will open a ticket to do this refactoring in one go for all other strategies if it does not exist.

Definitely! IMO we should do it in the frame of https://gridsingularity.atlassian.net/browse/GSYE-769.

spyrostz · 2024-10-01T11:08:58Z

src/gsy_e/models/strategy/storage.py

        self.cap_price_strategy = cap_price_strategy
        self.balancing_energy_ratio = BalancingRatio(*balancing_energy_ratio)

+    @staticmethod
+    def deserialize_args(constructor_args: Dict) -> Dict:
+        """Deserialize the constructor arguments for the HeatPump strategy."""


Minor

Suggested change

"""Deserialize the constructor arguments for the HeatPump strategy."""

"""Deserialize the constructor arguments for the Storage strategy."""

spyrostz · 2024-10-01T11:10:03Z

src/gsy_e/models/strategy/storage.py

        energy_rate_decrease_per_update=None,
        update_interval=None,
        initial_energy_origin: Enum = ESSEnergyOrigin.EXTERNAL,
+        losses: Optional[StorageLosses] = StorageLosses(),


Can be None here too AFAIS.

spyrostz · 2024-10-01T11:10:35Z

src/gsy_e/models/strategy/storage.py

        self.cap_price_strategy = cap_price_strategy
        self.balancing_energy_ratio = BalancingRatio(*balancing_energy_ratio)

+    @staticmethod
+    def deserialize_args(constructor_args: Dict) -> Dict:


I think that this method only needs to be implemented if we want to involve gsy-web. Since we do not this could be completely omitted. That said, no problem keeping it in case we need it in the future.

OK, let me remove it here, only to explicitly add it once/if we add the losses to the FE.

spyrostz

Some comments, LGTM in general, thanks!

spyrostz

LGTM

GSYE-645: Add loss calculation to storage state

dd3438e

hannesdiedrich requested a review from BigTava September 26, 2024 05:44

GSYE-645: Reverted changes in order to fix tests

d3de5b9

hannesdiedrich commented Sep 26, 2024

View reviewed changes

BigTava previously approved these changes Sep 26, 2024

View reviewed changes

GSYE-645: Remove unneeded prints

963c473

hannesdiedrich dismissed BigTava’s stale review via 963c473 September 26, 2024 09:41

GSYE-645: Rename loss fields

e5bdc9c

spyrostz reviewed Oct 1, 2024

View reviewed changes

hannesdiedrich added 2 commits October 2, 2024 08:41

GSYE-645: Adapt to review comments

b2f713f

GSYE-645: Add loss_history to storage results csv

3c11349

spyrostz approved these changes Oct 2, 2024

View reviewed changes

Merge branch 'master' into feature/GSYE-645

66e2587

hannesdiedrich merged commit d52597c into master Oct 7, 2024
3 of 4 checks passed

hannesdiedrich deleted the feature/GSYE-645 branch January 21, 2025 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSYE-645: Add loss calculation to storage state #1799

GSYE-645: Add loss calculation to storage state #1799

hannesdiedrich commented Sep 26, 2024

hannesdiedrich Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

BigTava left a comment •

edited

Loading

BigTava Sep 26, 2024

hannesdiedrich Sep 26, 2024

BigTava Sep 26, 2024

hannesdiedrich commented Sep 26, 2024

spyrostz Oct 1, 2024

hannesdiedrich Oct 2, 2024

spyrostz Oct 1, 2024

spyrostz Oct 1, 2024

hannesdiedrich Oct 2, 2024

hannesdiedrich Oct 2, 2024

spyrostz Oct 2, 2024

spyrostz Oct 1, 2024

hannesdiedrich Oct 2, 2024

spyrostz Oct 1, 2024

spyrostz Oct 1, 2024

spyrostz Oct 1, 2024 •

edited

Loading

hannesdiedrich Oct 2, 2024

spyrostz left a comment

spyrostz left a comment

-        losses: Optional[StorageLosses] = StorageLosses(),
-    ):
+        losses: Optional[StorageLosses] = None,
+    ):
+        if not losses:
+            losses = StorageLosses()

	"""Deserialize the constructor arguments for the HeatPump strategy."""
	"""Deserialize the constructor arguments for the Storage strategy."""

GSYE-645: Add loss calculation to storage state #1799

GSYE-645: Add loss calculation to storage state #1799

Conversation

hannesdiedrich commented Sep 26, 2024

Reason for the proposed changes

Proposed changes

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

BigTava left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannesdiedrich commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyrostz Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyrostz left a comment

Choose a reason for hiding this comment

spyrostz left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 •

edited

Loading

BigTava left a comment •

edited

Loading

spyrostz Oct 1, 2024 •

edited

Loading