Skip to content

fix(svdsbtl): skip patch polish for two-phase points#2949

Merged
ibell merged 2 commits into
masterfrom
ihb/svdsbtl-skip-polish-twophase
May 22, 2026
Merged

fix(svdsbtl): skip patch polish for two-phase points#2949
ibell merged 2 commits into
masterfrom
ihb/svdsbtl-skip-polish-twophase

Conversation

@ibell
Copy link
Copy Markdown
Contributor

@ibell ibell commented May 22, 2026

Summary

polish_patch_state_ runs a forward-h TOMS748 polish on every HmassP / HmolarP probe that lands inside the critical-patch bbox, on the assumption that a single-phase solution exists at h(T, p) = h_target on a tight T bracket around T_seed. Inside the saturation dome h(T, p) is multi-valued (it jumps from h_L to h_V across T = T_sat), so the bracket [T_seed-0.5, T_seed+0.5] straddles the dome and resid changes sign without any single-phase root in the range. TOMS748 silently converges to a nonsense T, and the returned state classifies as supercritical_liquid with rho / Q = ±inf.

The auto-calibrated patch bbox for non-water fluids whose critical region overlaps a wide h-range (R245fa, R134a, …) has its HmassP envelope straddle the saturation dome by construction, so any two-phase probe inside the patch hits the failure mode.

This skips the polish entirely when the patched state is iphase_twophase — two-phase points coming out of the patch already have correct (T_sat, Q, rho) from the source's PQ-blend; the forward-h polish was only designed for the ±25 mK backward-equation residual of IF97's single-phase formulations.

Reproducer (R245fa, before the fix):

HmassP probe: h=450.7e3 J/kg, p=2.89e6 Pa   (Q=0.5 dome point)
HEOS truth :  rho = 346.65, phase = twophase, Q = 0.500
SVDSBTL    :  rho = -inf,   phase = sc_liq,   Q = -inf

After the fix all 6 sweep probes (T = 0.96 … 0.999 · Tc, Q = 0.5) match HEOS within ~1e-14.

Test plan

  • CatchTestRunner "[SVDSBTL]" — 40 passed / 6 skipped / 0 failed
  • Spot-checked R245fa near-critical dome at T/Tc ∈ {0.96, 0.97, 0.98, 0.99, 0.995, 0.999} — all within ~1e-14 of HEOS
  • No regressions on single-phase in-patch points (polish path still runs)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Avoided applying single-phase polishing to points originating from two-phase sources, reducing incorrect or multi-valued results near saturation boundaries.
    • Patched points now reliably inherit and report their source phase after updates, with added safeguards for sources that may not expose phase information.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f73eda43-84d3-4aee-8389-6297c1bbd425

📥 Commits

Reviewing files that changed from the base of the PR and between ae72c74 and ef6fb1a.

📒 Files selected for processing (1)
  • src/Backends/SVDSBTL/SVDSBTLBackend.cpp

📝 Walkthrough

Walkthrough

Adds phase-aware guards to skip forward-h polishing when the patch source is two-phase, and sets a patched point's phase from patch_source->phase() (with try/catch) instead of leaving it as not-imposed.

Changes

Two-phase critical-patch polishing guards & phase propagation

Layer / File(s) Summary
Guard polish_patch_state_ for two-phase sources
src/Backends/SVDSBTL/SVDSBTLBackend.cpp
Wraps calls to polish_patch_state_ in patch_source_->phase() != iphase_twophase checks for HmolarP_INPUTS (via HmassP_INPUTS using *pt.h_mass, *pt.p) and HmassP_INPUTS; PT_INPUTS behavior unchanged.
Set PointEvaluation _phase from patch source
src/Backends/SVDSBTL/SVDSBTLBackend.cpp
When update() resolves a point as PointEvaluation::Kind::Patched, sets _phase = patch_source_->phase() inside a try/catch instead of leaving _phase as iphase_not_imposed on success.

Poem

I’m a rabbit in CoolProp’s glade,
Skipping polish where two-phase is made,
I pass along the phase with care,
So patched points know what they bear—
Hops, checks, and tidy state declared. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly matches the main change: skipping patch polish for two-phase points in the SVDSBTL backend.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ihb/svdsbtl-skip-polish-twophase

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/Backends/SVDSBTL/SVDSBTLBackend.cpp`:
- Around line 541-546: The patched branch preserves a genuine two-phase patch in
patch_source_ but update() fails to propagate that into the internal _phase,
leaving _phase as iphase_not_imposed; inside the PointEvaluation::Kind::Patched
handling (where only iQ is copied currently) update the code to also copy
patch_source_->phase() into _phase (and any other phase-tracking members used by
state.phase()), so that state.phase() matches the preserved patch_source_ phase
after polish_patch_state_ and in the subsequent logic that relies on _phase.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b97efb04-709e-4b02-8d06-77747fb1ddbf

📥 Commits

Reviewing files that changed from the base of the PR and between 9e57158 and 330dd04.

📒 Files selected for processing (2)
  • .beads/issues.jsonl
  • src/Backends/SVDSBTL/SVDSBTLBackend.cpp

Comment thread src/Backends/SVDSBTL/SVDSBTLBackend.cpp
ibell added a commit that referenced this pull request May 22, 2026
…aries

Two combined effects vs the previous plot:
 - SA-backed sat boundary curves replace the 64-knot cubic spline in
   the atlas, so liquid-side error drops by ~100x for R245fa and
   ~3000x for Water (median rho rel-err 5.33e-12 vs old 1.61e-08).
 - The patch-polish bug fix (PR #2949) restores correctness for the
   previously-missing wedge of two-phase points inside the critical
   patch bbox -- those cells now blend correctly through HEOS PQ
   instead of returning rho=-inf via the corrupted polish state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ibell added a commit that referenced this pull request May 22, 2026
Follow-up to the polish-skip-for-twophase fix: the Kind::Patched
branch in update() was copying iQ from patch_source_ but leaving
_phase as iphase_not_imposed.  For genuine two-phase patched
states (which the skip-polish path now correctly preserves) that
meant state.phase() disagreed with the source — rho / Q were
correct but the caller saw iphase_not_imposed instead of
iphase_twophase, surfacing as e.g. phase=8 (sc_liq) on R245fa
near-critical dome probes.

Read patch_source_->phase() and write it into _phase alongside the
iQ copy.  Defensive try/catch around the phase read mirrors the
existing iQ pattern in case a source backend doesn't always report
a phase value.

Resolves CodeRabbit's review comment on PR #2949 — without this
follow-up the polish skip is correct for rho/T/Q but the phase
remains stale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ibell and others added 2 commits May 22, 2026 12:17
polish_patch_state_ assumes a single-phase solution exists at
h(T, p) = h_target on a tight T bracket around T_seed.  Inside the
saturation dome h(T, p) is multi-valued (jumps from h_L to h_V across
T = T_sat), so the bracket [T_seed-0.5, T_seed+0.5] straddles the
dome and resid changes sign without any single-phase root in the
range.  TOMS748 silently converges to a nonsense T and the returned
state classifies as supercritical_liquid with rho / Q = ±inf.

Skip the polish entirely when the patched state is iphase_twophase
— two-phase points coming out of the patch already have correct
(T_sat, Q, rho) from the source's PQ-blend; the forward-h polish
was only designed for the ±25 mK backward-equation residual of
IF97's single-phase formulations.

Reproducer (R245fa, before the fix):
  h=450.7e3 J/kg, p=2.89e6 Pa  (Q=0.5 dome point inside auto-cal
  patch bbox) → rho=-inf, phase=supercritical_liquid, Q=-inf
  HEOS:                                                  rho=346.65
After:                                                   rho=346.65

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the polish-skip-for-twophase fix: the Kind::Patched
branch in update() was copying iQ from patch_source_ but leaving
_phase as iphase_not_imposed.  For genuine two-phase patched
states (which the skip-polish path now correctly preserves) that
meant state.phase() disagreed with the source — rho / Q were
correct but the caller saw iphase_not_imposed instead of
iphase_twophase, surfacing as e.g. phase=8 (sc_liq) on R245fa
near-critical dome probes.

Read patch_source_->phase() and write it into _phase alongside the
iQ copy.  Defensive try/catch around the phase read mirrors the
existing iQ pattern in case a source backend doesn't always report
a phase value.

Resolves CodeRabbit's review comment on PR #2949 — without this
follow-up the polish skip is correct for rho/T/Q but the phase
remains stale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ibell ibell force-pushed the ihb/svdsbtl-skip-polish-twophase branch from ae72c74 to ef6fb1a Compare May 22, 2026 16:17
@ibell ibell merged commit f7c9f63 into master May 22, 2026
21 of 22 checks passed
@ibell ibell deleted the ihb/svdsbtl-skip-polish-twophase branch May 22, 2026 16:18
ibell added a commit that referenced this pull request May 22, 2026
…aries

Two combined effects vs the previous plot:
 - SA-backed sat boundary curves replace the 64-knot cubic spline in
   the atlas, so liquid-side error drops by ~100x for R245fa and
   ~3000x for Water (median rho rel-err 5.33e-12 vs old 1.61e-08).
 - The patch-polish bug fix (PR #2949) restores correctness for the
   previously-missing wedge of two-phase points inside the critical
   patch bbox -- those cells now blend correctly through HEOS PQ
   instead of returning rho=-inf via the corrupted polish state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ibell ibell added this to the v8.0.0 milestone May 27, 2026
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.

1 participant