Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1915 +/- ##
=======================================
Coverage 71.13% 71.14%
=======================================
Files 149 149
Lines 14302 14309 +7
Branches 1889 1890 +1
=======================================
+ Hits 10174 10180 +6
- Misses 3589 3590 +1
Partials 539 539 🚀 New features to boost your workflow:
|
… from electricity if the solver does not return a valid solution
hannesdiedrich
force-pushed
the
feature/GSYE-901
branch
from
2dab3cc to
6ec459b
Compare
…delFitter in order to fit to the way of calling it in the gsy-e
| condenser_temp_C: float, | ||
| heat_demand_kW: Optional[float], | ||
| electrical_demand_kW: Optional[float] = None, | ||
| heat_demand_kW: Optional[float] = None, |
There was a problem hiding this comment.
I do not understand the None default value here. Does this method expect to work without heat demand as input ? I would expect not.
There was a problem hiding this comment.
It is because the Universal model does not use the heat demand:
https://github.com/gridsingularity/gsy-e/pull/1915/changes/BASE..d68c0aaefb2328cc68fef8c3312836cb4dc83689#diff-ffc30e0a7780c0a4a9c07a98677f476d25298f795edb3ee87ba8307679562a56L255
There was a problem hiding this comment.
Got it, then ignore this and the following comment
| self, | ||
| source_temp_C: float, | ||
| condenser_temp_C: float, | ||
| heat_demand_kW: Optional[float] = None, |
There was a problem hiding this comment.
Similar to a previous comment, I think that heat demand should not be optional, because of this assertion later on:
assert heat_demand_kW is not None, "heat demand should be provided"
| if bought_energy_kWh < FLOATING_POINT_TOLERANCE: | ||
| self._hp_state.set_cop(time_slot, self._hp_state.get_cop(last_time_slot)) | ||
| return |
There was a problem hiding this comment.
For my understanding, this means that if the heat pump did not buy any energy at all, then the COP value will be equal to the value of the last time slot ? This is a bit misleading in the graphs, since this is usually reported as 0 COP. Since we use the last slot COP though in case the COP model cannot calculate the COP for a specific heat demand, I agree to keep it because a non-zero last slot COP will be required in these cases.
There was a problem hiding this comment.
Yes, you understand correctly.
It is indeed not ideal and the cop graphs look weird. Especially, if you compare the same simulation setup with different size of storages.
I had an alternative implemented, but did not push the changes:
If the Universal COP model is selected, we could still calculate a COP value here, because it does not need a traded energy value. This will lead to another if block here that checks for the COP model and if set to "Universal" we could calculate the COP value from the temperatures. If not, we return the latest value.
What do you think?
There was a problem hiding this comment.
I get your point for the alternative implementation but I would leave it as is in order to not further complicate the code for this cornercase. Thanks for the clarification!
spyrostz
left a comment
spyrostz
left a comment
There was a problem hiding this comment.
Took a while, bit the PR is straightforward. LGTM
2 participants
Reason for the proposed changes
Please describe what we want to achieve and why.
Proposed changes
INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=master
SCM_ENGINE_BRANCH=master