Feature/gsye 651 #1703
Feature/gsye 651 #1703
Conversation
…to the market maker rate in the trading strategies
…egy.deserialize_args in order to trigger the extraction of FIT and MMR aka default values.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1703 +/- ##
==========================================
- Coverage 68.73% 68.59% -0.14%
==========================================
Files 142 143 +1
Lines 13447 13467 +20
Branches 2014 2014
==========================================
- Hits 9243 9238 -5
- Misses 3694 3715 +21
- Partials 510 514 +4 |
| update_interval=(duration( | ||
| minutes=constructor_args.get("update_interval")) | ||
| if constructor_args.get("update_interval") else None), |
There was a problem hiding this comment.
This clause is only in order to avoid update_interval=0, right ? Does not hurt to revert to None however I am not certain whether this is preferable to let the validator handle and reject this value. Leave it as is at any case.
There was a problem hiding this comment.
Ah, I did not think about the update_interval=0 case. You are right, the validator should handle this, but AFAIS, it does not handle it (https://github.com/gridsingularity/gsy-framework/blob/master/gsy_framework/validators/heat_pump_validator.py#L68).
The real fix would be to change this here to
| update_interval=(duration( | |
| minutes=constructor_args.get("update_interval")) | |
| if constructor_args.get("update_interval") else None), | |
| update_interval=(duration( | |
| minutes=constructor_args.get("update_interval")) | |
| if constructor_args.get("update_interval") is not None else None), |
And add a check in the validator for 0 seconds.
I propose tot fix this like this. Do you agree?
There was a problem hiding this comment.
Sorry missed this, agreed of course and thanks!
| @@ -259,11 +254,8 @@ def area_reconfigure_event(self, *args, **kwargs): | |||
|
|
|||
| def event_activate_price(self): | |||
| """Update the strategy prices upon the activation and validate them afterwards.""" | |||
| # If use_market_maker_rate is true, overwrite final_buying_rate to market maker rate | |||
| if self.use_market_maker_rate: | |||
There was a problem hiding this comment.
Why not including the use_market_maker_rate check in the mixin too ? That way it might be easier in the future if we revert from inheritance to composition (convert the mixin to a normal class that will be constructed in the LoadHoursStrategy constructor)
There was a problem hiding this comment.
Makes sense, will do.
| @@ -44,15 +44,15 @@ def __init__(self, energy_sell_rate=None, energy_rate_profile=None, energy_buy_r | |||
| # buy | |||
| if all(arg is None for arg in [ | |||
| buying_rate_profile, buying_rate_profile_uuid, energy_buy_rate]): | |||
| energy_buy_rate = ConstSettings.GeneralSettings.DEFAULT_MARKET_MAKER_RATE | |||
| energy_buy_rate = GlobalConfig.FEED_IN_TARIFF | |||
There was a problem hiding this comment.
I am hesitant to have yet another round for this, but I think that since this parameter is mutable, it is better to follow the same approach as for the MARKET_MAKER_RATE. Essentially to create a new constant named DEFAULT_FEED_IN_TARIFF, and in the GlobalConfig class, to assign it FEED_IN_TARIFF = ConstSettings.DEFAULT_FEED_IN_TARIFF. That way the default value will remain unchanged, even if the GlobalConfig.FEED_IN_TARIFF changes.
| self._buy_energy_profile = EnergyProfile( | ||
| buying_rate_profile, buying_rate_profile_uuid, energy_buy_rate, | ||
| profile_type=InputProfileTypes.IDENTITY) | ||
|
|
||
| # sell | ||
| if all(arg is None for arg in [ | ||
| energy_rate_profile, energy_rate_profile_uuid, energy_sell_rate]): | ||
| energy_sell_rate = ConstSettings.GeneralSettings.DEFAULT_MARKET_MAKER_RATE | ||
| energy_sell_rate = GlobalConfig.MARKET_MAKER_RATE |
There was a problem hiding this comment.
Why not keep using the default here? The MARKET_MAKER_RATE is mutable and changes throughout the simulation.
tests/strategies/test_heat_pump.py
Outdated
| @@ -37,6 +39,7 @@ def fixture_heatpump_strategy(request) -> Tuple["TradingStrategyBase", "Area"]: | |||
| gsye_root_path, "resources", "hp_external_temp_C.csv"), | |||
| **strategy_params) | |||
| else: | |||
| print(strategy_params) | |||
There was a problem hiding this comment.
I think that this can be removed.
hannesdiedrich
force-pushed
the
feature/GSYE-651
branch
from
1049a62 to
6306867
Compare
Before I looked into the HP, I added a mixin for the market maker rate and only realised afterwards, that in the heat pump we deal with the price settings differently.
Only a little tweak had to be done in the HP in order for the FE to just set the final buying rate in the heat pump which leads to using the market maker rate.
INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=feature/GSYE-651