-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix hp results leading to overheating house and other effects #853
base: dev
Are you sure you want to change the base?
Conversation
Description what caused the error / wrong behaviour Heat pump not under control of EmAgent point 1When the lower temperature limit of the heat house is reached, the heat pump starts to operate, but not immediately when the limit is reached. This is caused by the fact that the maybeThreshold is calculated correctly, but is not added to the additionalActivationTicks. to be updated |
This reverts commit 374a2ee.
…een charged within the last state
Description what caused the error / wrong behaviour part 2 House got overheated again Point 4Once the house has reached the upper temperature level, the heat from the HP is used to charge the thermal storage. At the next arrival of weather data, the thermal house will be triggered again to determine an updated state. In the current version of With the changes proposed so far, the hp will now run to heat the house first, then charge the storage, but will continue to run as the house cools down while the storage is being charged, and will therefore have a possible additional heat demand which will then be covered by the hp. Therefore, the demand calculation for the next state needs to be adjusted to only have an additional demand when the inner temperature is below the target temperature. |
This reverts commit a3959cb.
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.
Some small question.
house | ||
.zip(state.houseState) | ||
.map { case (house, state) => | ||
house.energyDemand( | ||
tick, | ||
ambientTemperature, | ||
state, | ||
) |
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.
Would it be possible to use the above thermalHouse
and updatedState
directly here?
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.
Good question but this brought me to another point which is a very good catch!
I guess, this is not correct. But please double check as well: The part above updates the lastState to the current tick. To determine the energyDemand from that tick on, one needs to use the updatedState, not the (last)state. Or?
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 think you need the updated state if you want to determine the current and future energy demand. If for example the house was heated up before and you use the state that does not include this information, it could lead to the overheating of the house.
Therefore I think you need the current state that includes the current temperature.
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 agree. Guess there might be improvement labeling the states to avoid these kind of failures. I changed to the updatedState already.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
// FIXME this is also in state -> refactoring by HiWi | ||
lastAmbientTemperature: Temperature, | ||
actualAmbientTemperature: Temperature, |
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.
Note from my side: In my opinion both temperatures are necessary here, the ambientTemperature of the last state and the actual one. Since the HpState contains the lastAmbientTemperature this can be done nicer and would be a good HiWi-Job. As well some tests for different cases like falling and rising temperature would be great.
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.
Transfered this into #916
# Conflicts: # CHANGELOG.md # src/main/scala/edu/ie3/simona/agent/participant/hp/HpAgentFundamentals.scala
# Conflicts: # CHANGELOG.md
resolves #827
merge #845 first!