Skip to content

fix HeatExchangerNetwork cache_network: keep stored order, match units by identity - #253

Merged
yoelcortes merged 3 commits into
masterfrom
fix-hxn-cache-network
Aug 28, 2026
Merged

fix HeatExchangerNetwork cache_network: keep stored order, match units by identity#253
yoelcortes merged 3 commits into
masterfrom
fix-hxn-cache-network

Conversation

@sarangbhagwat

Copy link
Copy Markdown
Member

Problem

cache_network=True on HeatExchangerNetwork was silently broken in two independent ways in HeatExchangerNetwork._cost():

  1. It never fired when auxiliary heat exchangers share IDs — the norm in biorefineries (several condenser/reboiler units). The cache-hit test compared sorted(hxs, key=ID) of the current utilities (unit order) with the stored list (duty-sorted synthesis order); ties keep input order, so the two lists differed and the network was re-synthesized every simulation.
  2. When it did fire (unique IDs), it paired the wrong streams. It rebuilt hxs in unit order and paired hxs[i] with stream_life_cycles[i], which synthesize_network built for the duty-sorted list. Each life cycle was seeded with the wrong stream. The final consistency check compared against the same wrongly paired unit, so it passed: every HXprocess ended with Q = 0, the heat-utility ratio went to 1.0, and the incremental HX cost collapsed — with no warning (class doctest system: 0.82 → 1.00).

Fix

Treat the stored synthesis order as the source of truth. The cache-hit test compares identity sets of units (HeatUtility objects are recreated on every simulation, so the unit is the only stable key), and the cached branch iterates self.original_heat_exchangers in stored order, looking up each unit's live HeatUtility. All downstream index-based pairing is then correct by construction.

Also:

  • the bare except: around the cache consistency check now catches AssertionError and reports the reason;
  • _energy_balance_error_contributions no longer raises TypeError when ignored is None.

Validation

  • New tests/test_hxn.py: the cache fires and matches fresh synthesis on the doctest system, after a 1 % feed perturbation, and with duplicate auxiliary IDs; ignored=None smoke test.
  • Sugarcane biorefinery with all 10 utility streams: cached HXN runs in 0.11 s vs 0.31 s, identical results.
  • HeatExchangerNetwork class doctest output unchanged.
  • Full suite (pytest . --disable-numba=1 -m "not slow"): same pre-existing failure set as on unmodified master (thermosteam/biorefineries stack drift, missing optional deps, tutorial notebooks); nothing new.

🤖 Generated with Claude Code

…s by identity

`cache_network=True` was silently broken in two independent ways in
`HeatExchangerNetwork._cost()`:

1. It never fired when auxiliary heat exchangers share IDs (the norm in
   biorefineries: several 'condenser'/'reboiler' units). The cache-hit test
   compared `sorted(hxs, key=ID)` of the current utilities (unit order) with
   the stored list (duty-sorted synthesis order); ties keep input order, so
   the two lists differed and the network was always re-synthesized.

2. When it did fire (unique IDs), it rebuilt `hxs` in unit order and paired
   `hxs[i]` with `stream_life_cycles[i]`, which `synthesize_network` built
   for the duty-sorted list. Each life cycle was seeded with the wrong
   stream. The final consistency check compared against the same wrongly
   paired unit, so it passed: every HXprocess ended with Q = 0, the heat
   utility ratio went to 1.0 and the incremental HX cost collapsed, with no
   warning (doctest system: 0.82 -> 1.00).

Fix: treat the stored synthesis order as the source of truth. The cache-hit
test compares identity sets of units (HeatUtility objects are recreated on
every simulation, so the unit is the only stable key), and the cached
branch iterates `self.original_heat_exchangers` in stored order, looking up
each unit's live HeatUtility. All downstream index-based pairing is then
correct by construction. Also: the bare `except:` around the cache
consistency check now catches `AssertionError` and reports the reason, and
`_energy_balance_error_contributions` no longer raises TypeError when
`ignored is None`.

Validation: new tests/test_hxn.py (cache fires and matches fresh synthesis
on the doctest system, after a 1% feed perturbation, and with duplicate
auxiliary IDs; `ignored=None` smoke test). On the sugarcane biorefinery with
all 10 utility streams the cached HXN now runs in 0.11 s vs 0.31 s with
identical results. Class doctest output unchanged. Full suite: same
pre-existing failures as the recorded baseline plus test_tire_modeling,
which fails identically on unmodified master (thermosteam-clone drift).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Previous comments are relevant for the improvements and review, but not necessarily for future readers.
Also remove Yoel's copyright (edits/contributions to the module were minor)

@yoelcortes yoelcortes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice improvements and great tests! I made only minor update to comments and increased tolerance for tests for reproducibility on my side (and potentially other machines).

@yoelcortes
yoelcortes merged commit 6321496 into master Aug 28, 2026
0 of 2 checks passed
sarangbhagwat added a commit that referenced this pull request Sep 4, 2026
Upstream merged the HXN PR stack (#253-#256) into master and added two
follow-up commits touching files this branch deleted when the heat
exchanger network moved to hensmith:

- bc06684 reworded the cache_network comments in
  biosteam/facilities/hxn/_heat_exchanger_network.py
- 15f6c63 loosened the cache_network test tolerance for reproducibility
  in tests/test_hxn.py (rtol 1e-6 -> 2e-3, feed perturbation 1.01 ->
  1.000001)

Both modify/delete conflicts are resolved by keeping the deletions; the
two changes are ported to hensmith (hensmith/_heat_exchanger_network.py
and tests/test_hxn.py) on its port-upstream-hxn-review-tweaks branch.
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.

2 participants