-
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
GSYE-568: Change hierarchy self consumption into percentage values #1800
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1800 +/- ##
==========================================
- Coverage 69.58% 69.55% -0.03%
==========================================
Files 148 148
Lines 14039 14055 +16
Branches 2623 2622 -1
==========================================
+ Hits 9769 9776 +7
- Misses 3752 3760 +8
- Partials 518 519 +1 |
len(hierarchy_self_consumption) > 0 | ||
and hierarchy_self_consumption[next(iter(hierarchy_self_consumption))] == 0 | ||
): | ||
# early return in case there is no self consumpton at all (in order to not devide by 0) |
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.
# early return in case there is no self consumpton at all (in order to not devide by 0) | |
# early return in case there is no self consumption at all (in order to not devide by 0) |
Nit
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.
assert isclose( | ||
sum(self.hierarchy_self_consumption_percent.values()), 100 | ||
), "Self consumption percentages do not sum up to 100%" |
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 a great assert, it will always confirm that this feature is working as expected, thanks!
|
||
def _calc_hierarchy_stats(self, area: "AreaBase", level: int, results: Dict): | ||
for level, self_consumption in hierarchy_self_consumption.items(): | ||
if level + 1 not in hierarchy_self_consumption: |
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.
Not that big of an issue, since it probably makes the solution simpler, however I think that hierarchy_self_consumption
could be a list and not a dict, since the index / position in the list could be used as the level number. Regardless if you believe it is more explicit that way leave it as is.
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 do not mind either way, so I changed it to be a list: 87ff961
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.
2 minor comments, LGTM
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
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=master