Feature/gsye 810 #1818
Feature/gsye 810 #1818
Conversation
…rts of the heat pumps because they are not needed
…to only have one source of truth
| @@ -28,59 +28,59 @@ def setup_method(self): | |||
| self._datetime = datetime(2023, 1, 1, 0, 0) | |||
|
|
|||
| def test_increase_tanks_temp_from_heat_energy(self): | |||
| self._tanks.increase_tanks_temp_from_heat_energy(1, self._datetime) | |||
| self._tanks.increase_tanks_temp_from_heat_energy(5000, self._datetime) | |||
There was a problem hiding this comment.
@spyrostz You need 4186 J to heat up one liter of water by 1 degree. Hence, in our cases where we talk about hundrets of liters we should change to use kJ as unit for the heat demand profile in order to not have too large numbers, for the input as well as for the output. What do you think?
If you agree, I will add a ticket for this.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1818 +/- ##
==========================================
+ Coverage 69.50% 69.56% +0.06%
==========================================
Files 148 150 +2
Lines 14181 14227 +46
Branches 2656 2666 +10
==========================================
+ Hits 9856 9897 +41
- Misses 3800 3801 +1
- Partials 525 529 +4 |
src/gsy_e/constants.py
Outdated
| TIME_FORMAT, | ||
| DATE_FORMAT, |
There was a problem hiding this comment.
Are these 2 and the NOQA required?
There was a problem hiding this comment.
I had to check myself again, too.
yes, we have a lot of imports in other modules that source these ones:
from gsy_e.constants import TIME_ZONEI am not sure whether this is good practice. Should I remove them and directly import from gsy-framework?
There was a problem hiding this comment.
I think that the best practice is to remove them from here actually and directly import them. I checked too and I think that the only reason why we had them like these were in order to use them as gsy_e.constants.TIME_FORMAT, however unless I do not see something, this can be replaced by a direct import. Therefore I would reecommend to take the time and remove them, and thanks!
| return 0 | ||
| electrical_power_kW = self._calc_power(source_temp_C, tank_temp_C, heat_demand_kW) | ||
| if electrical_power_kW <= 0: | ||
| log.debug( |
There was a problem hiding this comment.
I would write an error here, and also I would prepend the configuration / simulation id in case this error will be logged in deployed environment.
There was a problem hiding this comment.
This might clutter or logs, but OK, let's do it and see how often this happens.
| return 0 | ||
| cop = heat_demand_kW / electrical_power_kW | ||
| if cop > 6: | ||
| log.debug( |
There was a problem hiding this comment.
Same here, an error log and a reference to the configuration uuid will be nice to have
| return self._cop_model.calc_cop( | ||
| source_temp_C=self._source_temp_C.get_value(time_slot), | ||
| tank_temp_C=self._state.tanks.get_average_tank_temperature(time_slot), | ||
| heat_demand_kW=heat_demand_kW, |
There was a problem hiding this comment.
Nothing to do here, I took a long time reviewing whether this will break the existing strategies, but as far as I saw the old strategies should operate like before so all good!
Reason for the proposed changes
Please describe what we want to achieve and why.
Proposed changes
INTEGRATION_TESTS_BRANCH=feature/GSYE-810
GSY_FRAMEWORK_BRANCH=feature/GSYE-810