Skip to content

288 water to water heat pump bypass mode#344

Open
samvanderzwan wants to merge 10 commits intomainfrom
288-water-to-water-heat-pump-bypass-mode
Open

288 water to water heat pump bypass mode#344
samvanderzwan wants to merge 10 commits intomainfrom
288-water-to-water-heat-pump-bypass-mode

Conversation

@samvanderzwan
Copy link
Contributor

Unit test are not yet working but code seems to be working

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a bypass mode for the water-to-water heat pump / heat transfer asset and propagates related sign/setpoint and controller-path/factor changes through the controller layer, solver asset equations, and unit/integration tests.

Changes:

  • Add bypass_mode support to the solver HeatTransferAsset, with separate “normal” vs “bypass” equation generation.
  • Extend heat pump/controller setpoints to support bypass and primary-side mass-flow/pressure control, and adjust network factor/path handling.
  • Update a broad set of unit/integration tests to reflect new sign conventions, setpoint keys, and solver behavior.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
unit_test/solver/network/assets/test_heat_transfer_asset.py Updates expectations for equation selection/call counts and mass-flow setup in solver-level tests.
unit_test/integration/test_heat_transfer_asset.py Adjusts integration scenarios for updated mass-flow initialization and new prescribe flags.
unit_test/entities/test_production_cluster.py Adapts convergence test to new sign convention behavior.
unit_test/entities/test_heat_pump.py Updates heat pump tests for new sign conventions and additional solver state; currently misaligned with new required setpoint keys.
unit_test/entities/test_heat_exchanger.py Updates mass-flow setup and expected output sign.
unit_test/entities/test_ates_cluster.py Adjusts expected well temperatures due to changed ATES behavior.
unit_test/entities/controller/test_controller_new_class.py Updates expectations for factor_to_first_network now being a list of factors.
unit_test/entities/controller/test_controller_network.py Updates factor representation and storage setpoint expectations; adapts pressure-setting behavior.
src/omotes_simulator_core/solver/network/assets/heat_transfer_asset.py Implements bypass mode and refactors equation generation; changes flow-direction handling and electric power computation.
src/omotes_simulator_core/infrastructure/app.py Changes default simulation window/timestep and hardcodes a bypass test run + CSV export in __main__.
src/omotes_simulator_core/entities/network_controller.py Refactors factor/path application, storage charge/discharge logic, heat-transfer setpoints, and pressure-setting key selection.
src/omotes_simulator_core/entities/assets/utils.py Changes heat-demand↔mass-flow conversion to use internal energy difference instead of heat capacity approximation.
src/omotes_simulator_core/entities/assets/production_cluster.py Adjusts mass-flow calculation sign and convergence criterion.
src/omotes_simulator_core/entities/assets/heat_pump.py Adds bypass setpoint support, primary-side control flag, and updates setpoint parsing/sign conventions.
src/omotes_simulator_core/entities/assets/heat_exchanger.py Adjusts heat-loss sign convention in outputs.
src/omotes_simulator_core/entities/assets/demand_cluster.py Updates convergence check sign usage (now sensitive to negative allocations).
src/omotes_simulator_core/entities/assets/controller/controller_network.py Changes factor tracking to list-of-factors and uses a product; extends storage setpoints; updates pressure-setting API.
src/omotes_simulator_core/entities/assets/controller/controller_heat_transfer.py Adds bypass option and updates how primary/secondary heat demand is set (factor vs bypass behavior).
src/omotes_simulator_core/entities/assets/ates_cluster.py Updates temperature assignment and convergence logic; tweaks ROSIM initialization/run cadence; adds heat supplied computation.
src/omotes_simulator_core/entities/assets/asset_defaults.py Adds PROPERTY_BYPASS constant.
src/omotes_simulator_core/entities/assets/asset_abstract.py Removes solver_asset type annotation from the base abstract asset.
src/omotes_simulator_core/adapter/transforms/controller_mapper.py Changes “root network” selection for path computation and updates how paths are generated.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 96 to 100
self.pre_scribe_mass_flow_secondary = pre_scribe_mass_flow_secondary
# Define the flag that indicates whether the mass flow rate or the pressure is prescribed
# at the hot side of the heat pump
self.pre_scribe_mass_flow_primary = pre_scribe_mass_flow_secondary
# Define the mass flow rate set point for the asset on the secondary side
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre_scribe_mass_flow_primary is initialized from pre_scribe_mass_flow_secondary, so the primary-side control flag will always mirror the secondary-side flag. This prevents independently controlling primary mass-flow vs pressure (and is likely a copy/paste bug). Add a dedicated constructor parameter (or default) for pre_scribe_mass_flow_primary and assign it here.

Copilot uses AI. Check for mistakes.
# Assign setpoints to the HeatPump asset
self.temperature_in_primary = setpoints_primary[PRIMARY + PROPERTY_TEMPERATURE_IN]
if self.first_time_step or self.solver_asset.prev_sol[0] == 0.0:
self.temperature_in_primary = setpoints_primary[SECONDARY + PROPERTY_TEMPERATURE_IN]
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_set_setpoints_primary() reads setpoints_primary[SECONDARY + PROPERTY_TEMPERATURE_IN], but the required keys are defined with the PRIMARY prefix. This will raise KeyError (or use the wrong temperature) whenever primary and secondary temperatures differ. Use PRIMARY + PROPERTY_TEMPERATURE_IN here.

Suggested change
self.temperature_in_primary = setpoints_primary[SECONDARY + PROPERTY_TEMPERATURE_IN]
self.temperature_in_primary = setpoints_primary[PRIMARY + PROPERTY_TEMPERATURE_IN]

Copilot uses AI. Check for mistakes.
Comment on lines 154 to +158
setpoints = {
PRIMARY + PROPERTY_TEMPERATURE_IN: 273.15 + 10.0,
PRIMARY + PROPERTY_TEMPERATURE_OUT: 273.15 + 20.0,
PRIMARY + PROPERTY_HEAT_DEMAND: 300,
PROPERTY_SET_PRESSURE: False,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes PROPERTY_SET_PRESSURE, but _set_setpoints_primary() now requires PRIMARY + PROPERTY_SET_PRESSURE. Align the test setpoints dict with the required key to avoid a missing-key ValueError.

Copilot uses AI. Check for mistakes.
return abs(self.get_heat_supplied() - (-self.thermal_power_allocation)) < (
(-self.thermal_power_allocation) * 0.001
return abs(self.get_heat_supplied() - self.thermal_power_allocation) < (
self.thermal_power_allocation * 0.001
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convergence tolerance should be non-negative. Since consumer profiles can yield negative demands, self.thermal_power_allocation * 0.001 can become negative and make this check always fail. Use abs(self.thermal_power_allocation) * 0.001 for the tolerance.

Suggested change
self.thermal_power_allocation * 0.001
abs(self.thermal_power_allocation) * 0.001

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zou hier inderdaad niet nog een extra abs ergens omheen moeten?

Comment on lines +520 to +531
else:
if self.iteration_flow_direction_secondary == FlowDirection.ZERO:
pset_out = self.pressure_set_point_secondary
pset_in = self.pressure_set_point_secondary
else:
if self.iteration_flow_direction_secondary == FlowDirection.POSITIVE:
pset_out = self.pressure_set_point_secondary / 2
pset_in = self.pressure_set_point_secondary
else:
pset_out = self.pressure_set_point_secondary
pset_in = self.pressure_set_point_secondary / 2
equations.append(
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In bypass mode, pressure splitting depends on iteration_flow_direction_secondary, but that value is never recomputed inside get_equations_bypass(). This can become stale across timesteps/setpoint changes and lead to incorrect pset_in/pset_out selection. Recompute the iteration flow direction(s) at the start of bypass equation generation, similar to get_equations_normal().

Copilot uses AI. Check for mistakes.
Comment on lines 102 to 106
SECONDARY + PROPERTY_TEMPERATURE_IN: 273.15 + 15.0,
SECONDARY + PROPERTY_TEMPERATURE_OUT: 273.15 + 25.0,
SECONDARY + PROPERTY_HEAT_DEMAND: 310,
SECONDARY + PROPERTY_HEAT_DEMAND: -310,
PROPERTY_SET_PRESSURE: True, # Boolean value
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These secondary-setpoint tests no longer match the HeatPump API: _set_setpoints_secondary() now requires SECONDARY + PROPERTY_SET_PRESSURE and PROPERTY_BYPASS, but the test still passes PROPERTY_SET_PRESSURE and omits PROPERTY_BYPASS. Update the test inputs/expectations accordingly (including the sign convention changes in the implementation).

Copilot uses AI. Check for mistakes.
Comment on lines +66 to +70
result = run(r".\testdata\heat_pump_bypass.esdl")
t2 = datetime.now()

logger.info(f"Results dataframe shape=({result.shape})")
result.to_csv(r".\testdata\heat_pump_bypass_results.csv", index=False)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __main__ block is hardcoded to run heat_pump_bypass.esdl and write a CSV into ./testdata, which is a surprising side effect and can break when run from other working directories. Consider removing these hardcoded paths or gating them behind explicit CLI args/flags.

Copilot uses AI. Check for mistakes.
Comment on lines 17 to 21
import datetime

from numpy.ma.core import product

from omotes_simulator_core.entities.assets.asset_defaults import (
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing product from numpy.ma.core relies on NumPy’s internal/private module structure. Prefer numpy.prod(...) or math.prod(...) (and import from the public namespace) to avoid breakage across NumPy versions.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just import numpy or import math.prod.

\dot{m}_{0} + \dot{m}_{1} = 0

If the mass flow at the inflow node of the primary and secondary side is not zero, we
prescribe the follwoing energy balance equation for the heat transfer asset:
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in docstring: "follwoing" → "following".

Suggested change
prescribe the follwoing energy balance equation for the heat transfer asset:
prescribe the following energy balance equation for the heat transfer asset:

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@vanmeerkerk vanmeerkerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er is erg veel gewijzgd waarvan sommige onderdelen naar mijn mening een apart issue hadden moeten worden. Over het algemeen ziet het er oké uit nog wat docstring opmerkingen.

Ik vermoed wel dat er nu een fout wordt gemaakt in het heat_transfer asset met de huidige implementatie.

SECONDARY + PROPERTY_TEMPERATURE_OUT: 273.15 + 80,
SECONDARY + PROPERTY_TEMPERATURE_IN: 273.15 + 50,
PROPERTY_SET_PRESSURE: False,
if bypass:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik heb hier wel een vraag over. Zetten we nu standaard temperaturen in dit asset met set_asset of is dit alleen een initialisatie stap?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Klopt de volgorder van de temperaturen; ik zou verwachtten dat PRIM_OUT gleijk moet zijn aan SEC_IN. Dit lijkt nu niet het geval?

Comment on lines 17 to 21
import datetime

from numpy.ma.core import product

from omotes_simulator_core.entities.assets.asset_defaults import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just import numpy or import math.prod.

for storage in self.storages:
storage_settings[storage.id] = {
PROPERTY_HEAT_DEMAND: +1 * storage.effective_max_charge_power * factor,
PROPERTY_TEMPERATURE_OUT: storage.temperature_out,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waarom heb je de temperatuur nodig voor een storage? Deze wordt toch niet geset maar gebasseerd op de internal state?

@@ -45,8 +45,6 @@ class AssetAbstract(ABC):

connected_ports: list[str]
"""List of ids of the connected ports."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je verwijdert solver_asset: Waarom? Alles refereert intern nog steeds naar solver_asset...

@@ -171,23 +169,30 @@ def set_setpoints(self, setpoints: dict) -> None:
setpoints_set = set(setpoints.keys())
# Check if all setpoints are in the setpoints
if necessary_setpoints.issubset(setpoints_set):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Persoonlijk vind ik de -1 * explicieter, waardoor het direct zichtbaar is dat er nog iets met de waarde gebeurt. De - is te missen.

self.flow_direction_secondary = self.flow_direction(
self.mass_flow_rate_rate_set_point_secondary
)
self.flow_direction_primary = self.flow_direction(self.prev_sol[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiermee bypass je wel je initiele setpoints; is dat handig?

@@ -270,7 +281,7 @@ def get_equations(self) -> list[EquationObject]:
) = self.get_ordered_connection_point_list()

if np.all(np.abs(self.prev_sol[0:-1:3]) < MASSFLOW_ZERO_LIMIT):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Klopt het nu dat we alleen de richting zetten op basis van de vorige oplossing als de stroming 0 was (< MASSFLOW_ZERO_LIMIT); ik vraag mij af of dit goed gaat.

if iteration_flow_direction_secondary == FlowDirection.ZERO:
mset = 0.0
if self.iteration_flow_direction_secondary == FlowDirection.ZERO:
mset = self.mass_flow_rate_rate_set_point_secondary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dus als 0 forceren we niet langer 0 maar gaan we terug naar het setpoint; dit kan niet juist zijn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Als je dit doet kan je net zo goed deze if loop weghalen en altijd het setpoint zetten. Ik denk dat je hier een fout maakt om het te laten werken. Ik herinner mij dat het zetten van mset=0.0 resulteerde in een matrix fout maar dat zit eerder in hoe je je setpoints zet op basis van een vorige oplossing.

if (self.iteration_flow_direction_primary != FlowDirection.ZERO) or (
self.iteration_flow_direction_secondary != FlowDirection.ZERO
):
equations.append(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hier gaat echt iets fout met het zetten vvan m=0.0

)
return equations

def set_internal_energy_equations_bypass(self, equations):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Water to water heat pump bypass mode

3 participants