-
Notifications
You must be signed in to change notification settings - Fork 21
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
Feature/gsye 766 #1798
Feature/gsye 766 #1798
Conversation
… sent by gsy-web - correctly get the energy rates from infinite bus asset in the first level of the scenario
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1798 +/- ##
=======================================
Coverage 69.52% 69.52%
=======================================
Files 148 148
Lines 14029 14029
Branches 2619 2619
=======================================
Hits 9753 9753
+ Misses 3759 3758 -1
- Partials 517 518 +1 |
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.
Forgot to submit my review here. LGTM once print removed.
@staticmethod | ||
def test_handle_non_p2p_scenario_adds_market_makers_to_homes(): | ||
handler = NonP2PHandler(SCENARIO) | ||
print(handler.non_p2p_scenario) |
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.
print spotted
if target_area.is_home_area and gsy_e.constants.RUN_IN_NON_P2P_MODE: | ||
area_dict["non_p2p"] = True |
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.
This is the only think that raises my eyebrow in this list of PRs actually. Not a problem since it is "only" in the results, however Not certain, however it might be better if we added a global constant as a part of the results. The disadvantage of this is that the detection of "home areas" will need to happen in gsy-framework, thus being slightly more complex than here. Thus, please ignore this suggestion and if we find the need for additional logic in the results service in the future, we can rethink about this approach.
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.
Yeah, I was not 100% happy with this implementation either. But you suggestion also does not convince me actually. I will take your offer to ignore it. Let's resume this discussion once we stumble over issues with this, ok?
|
||
|
||
class NonP2PHandler: | ||
"""Handles non-p2p case""" | ||
|
||
def __init__(self, scenario: dict): | ||
self.non_p2p_scenario = scenario | ||
self._energy_sell_rate = 0.0 | ||
self._energy_buy_rate = 0.0 | ||
self._get_energy_rates_from_infinite_bus(scenario) |
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.
For now this is good. However, keep in mind that in the future we might need to set different energy rates per home. We will deal with it when we need it though, nothing to do for now.
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.
Yes, having different rates per home will lead to more changes anyway. Let's deal with this then. Thanks for pointing out though.
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.
LGTM
Reason for the proposed changes
Please describe what we want to achieve and why.
Proposed changes
INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=feature/GSYE-766