-
Notifications
You must be signed in to change notification settings - Fork 0
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
41 store simulation data #211
base: main
Are you sure you want to change the base?
Conversation
Added writing of outputs to the output dataframe. |
def get_volume_flow_rate(self, i: int) -> float: | ||
"""Calculates and returns the volume flow rate for the given port. | ||
|
||
:param int i: The index of the port. | ||
:return float: The volume flow rate. | ||
""" | ||
rho = fluid_props.get_density(self.solver_asset.get_temperature(i)) | ||
return self.solver_asset.get_mass_flow_rate(i) / rho |
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.
Why a separate function and not integrated in write_standard_output. You're also calling functions to get massflow and temperature, which is already being done in write_standard_output()
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.
Because now it is clear what the purpose of this function is and we can test it. If you would put it in the write standard output it would be more difficult to test if the volume flow rate is calculated correctly
} | ||
self.outputs[1][-1].update(output_dict_temp) | ||
|
||
def get_actual_heat_supplied(self) -> float: |
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.
Why the name get_actual_heat_supplied()? this is just the heat supplied to the system right?
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 but you have the set point and the actual heat supplied, which might be different due to shortage of supply or due to temperature losses.
The later we should account for in future.
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.
So it's the heat_supplied
but the naming of the setpoint is wrong?
def get_heat_loss(self) -> float: | ||
"""Get the heat loss of the pipe. | ||
|
||
The minus sign is added to make it a loss instead of supply. | ||
""" | ||
return -self.solver_asset.heat_supplied |
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.
Heat supplied * -1 is heat loss? that's confusing.....
Why not call it heat flux in the solver_asset and then it's up to the entity to determine if it's supplied or lost.
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 because we have implemented the energy equation as a supply equation. But from user perspective it is more logical to talk about an heat loss for a pipe, hence the minus sign.
PROPERTY_PRESSURE_LOSS: self.get_pressure_loss(), | ||
PROPERTY_PRESSURE_LOSS_PER_LENGTH: self.get_pressure_loss_per_length(), | ||
PROPERTY_HEAT_LOSS: self.get_heat_loss(), |
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.
These functions should be computed locally. Now you're calling get_pressure 4 times, where only 2 is needed.
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.
What do you mean with locally?
def write_to_output(self) -> None: | ||
"""Placeholder to write the asset to the output. | ||
"""Method to write the asset to the output. |
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.
Confusing docstring, it doesn't match what the method actualy does. This method doesn't write to "the output" (i.e. console, logging.whatever). It updates an internal datastructure with the current timestep values.
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.
Adjusted
The output list is a list of dictionaries, where each dictionary | ||
represents the output of its asset for a specific timestep. | ||
""" |
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.
The docstring is just wrong. This is not a placeholder method! It also doesn't write to "output" (I would expect console output, file output, logging, whatever). It updates an internal datastructure with the values of the latest computed timestep values.
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.
Adjusted
for i in range(len(self.connected_ports)): | ||
output_dict_temp = {PROPERTY_VELOCITY: self.get_velocity(i)} | ||
if i == 1: # only for the second connection point these properties are added | ||
output_dict_temp.update( | ||
{ | ||
PROPERTY_PRESSURE_LOSS: self.get_pressure_loss(), | ||
PROPERTY_PRESSURE_LOSS_PER_LENGTH: self.get_pressure_loss_per_length(), | ||
PROPERTY_HEAT_LOSS: self.get_heat_loss(), | ||
} | ||
) | ||
self.outputs[i][-1].update(output_dict_temp) |
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.
Why the loop? This happens in the pipe asset, where we know 100% sure that this thing only has two ports right?
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.
Velocity is calculated on both ports. Adjusted to remove the if statement
@@ -309,3 +309,19 @@ def test_base_get_temperature(self) -> None: | |||
|
|||
# Assert | |||
self.assertAlmostEquals(result, temperature, 2) | |||
|
|||
def test_base_get_internal_energy(self) -> None: | |||
"""Test the get_internal_energy method.""" |
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.
redundant docstring. This tests whether the base_asset.get_internal_energy() method reads the value from the correct place in memory. This is validated by a manual array index computation on lines 318-321, which corresponds to the documentation of our memory structures (which we probably don't have yet, but okay...)
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.
Doc string is needed otherwise you get linting error on github
unit_test/entities/test_pipe.py
Outdated
# act | ||
velocity = self.pipe.get_velocity(port=0) | ||
# assert | ||
self.assertEqual(velocity, 12.732395447351628) |
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.
Please use assertAlmostEqual
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.
done
Fixed all issues also merge conflicts. @samvanderzwan can you check and if ok approve |
We need to discuss the order of calling write_standard_output and write_output, since this order is now required otherwise the write_output is writing away at wrong position.