-
Notifications
You must be signed in to change notification settings - Fork 7
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-651: Adapt heat pump validator to also allow parcial provision o… #480
Changes from all commits
c6eb2dd
1b563f2
3a66f36
697e444
717a7a6
725ea86
b17c96b
aa11104
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
from gsy_framework.constants_limits import ConstSettings | ||
from gsy_framework.constants_limits import ConstSettings, GlobalConfig | ||
from gsy_framework.exceptions import GSyDeviceException | ||
from gsy_framework.validators.base_validator import BaseValidator | ||
from gsy_framework.validators.utils import validate_range_limit | ||
|
@@ -67,20 +67,23 @@ def _validate_temp(cls, **kwargs): | |
@classmethod | ||
def validate_rate(cls, **kwargs): | ||
"""Validate energy rate related arguments.""" | ||
if not(kwargs.get("initial_buying_rate") | ||
and kwargs.get("final_buying_rate") | ||
and kwargs.get("update_interval")): | ||
if (kwargs.get("initial_buying_rate") is None | ||
and kwargs.get("final_buying_rate") is None | ||
and kwargs.get("update_interval") is None): | ||
Comment on lines
+71
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the fix, however I think that this is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After looking at the following clause though, I think that this clause is redundant, because we raise an exception if any of the 3 is None. I would recommend to remove this completely actually. |
||
return | ||
if (not kwargs.get("initial_buying_rate") | ||
or not kwargs.get("final_buying_rate") | ||
or not kwargs.get("update_interval")): | ||
if (kwargs.get("initial_buying_rate") is None | ||
or kwargs.get("update_interval") is None): | ||
raise GSyDeviceException( | ||
{"misconfiguration": [ | ||
"All pricing parameters of heat pump should be provided:" | ||
"initial_buying_rate, final_buying_rate, update_interval"]}) | ||
"initial_buying_rate, update_interval"]}) | ||
|
||
if kwargs.get("update_interval") == 0: | ||
raise GSyDeviceException( | ||
{"misconfiguration": ["update_interval should not be zero"]}) | ||
|
||
buying_rate_arg_names = [ | ||
"initial_buying_rate", "preferred_buying_rate", "final_buying_rate"] | ||
"initial_buying_rate", "preferred_buying_rate"] | ||
for buying_rate_arg_name in buying_rate_arg_names: | ||
cls._check_range( | ||
name=buying_rate_arg_name, value=kwargs[buying_rate_arg_name], | ||
|
@@ -89,7 +92,9 @@ def validate_rate(cls, **kwargs): | |
|
||
initial_buying_rate = kwargs["initial_buying_rate"] | ||
preferred_buying_rate = kwargs["preferred_buying_rate"] | ||
final_buying_rate = kwargs["final_buying_rate"] | ||
final_buying_rate = ( | ||
kwargs.get("final_buying_rate") if kwargs.get("final_buying_rate") | ||
else GlobalConfig.MARKET_MAKER_RATE) | ||
|
||
validate_range_limit( | ||
initial_buying_rate, preferred_buying_rate, final_buying_rate, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spyrostz Sorry to drag you into this one more time.
We need this clause in order to allow the user to not provide pricing at all and let the strategy use the default values.
For this case, we do not need to validate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rea-added it in the last commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and thank you for correcting the clause! It now is correct and reflects the condition of all being None, contrary to the former implementation which evaluated if one of them was None.