Repository navigation
Fix trace-water phase split causing pump PS-flash failures - #4320
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Non-finite residuals can bypass repair, and the regression test bypasses the public flash path.
2 open findings
What changed in this PR
Fixes trace-water phase-split recovery to prevent non-conservative states from breaking PS flashes.
Changes:
- Re-solves invalid aqueous phase fractions from a bounded 50/50 seed.
- Adds thermodynamic regression coverage.
- Regenerates engineering coverage metadata.
| File | Description |
|---|---|
TPflash.java |
Adds aqueous endpoint recovery logic. |
TPflashWaterBearingPhaseCollapseRecoveryTest.java |
Tests material and thermodynamic closure. |
engineering-coverage.json |
Updates the TPflash source digest. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Review of Recommendation: retain this focused repair, but request stronger public-path and performance qualification before merging. It repairs a real non-conservative endpoint without weakening PS tolerances. It is not yet evidence that the upstream phase-lifecycle cause is fixed, or that all flash workloads retain their speed. What I verifiedThe six workflow groups for this head are successful. I inspected the build jobs: Java 8/21 on Ubuntu/Windows, four slow-test shards, model specifications and Javadocs passed (run). I also compiled the exact base/head For the public synthetic heptane/water case at 307 K, 10.5 bara and 0.003 water mole fraction, after imposing the test's corrupted betas:
On the repaired states, maximum log-fugacity residual is below 1.31e-11; entropy and Gibbs energy return to the independently flashed reference within about 3.7e-12 J/K and 7.9e-11 J respectively. The 50/50 split is correctly used as a numerical seed. Corrections to the automated review
Changes/qualification requested for this PR1. Add a genuinely failing public TP → PS/pump reproducer. Keep the useful private regression. Merely replacing its reflective invocation with 2. Distinguish a conservative phase split from a stable phase set. The new branch (lines 2285–2298) repairs the existing two phases and returns before 3. Bound actual recovery work and make exhausted repair observable. Twenty calls to 4. Clarify tolerances and documentation. The reused production material-balance threshold is 1e-8, whereas the new test asserts 1e-10 and #2937 targets ordinary component balance at least that tight. Passing one test does not make the production gate a 1e-10 guarantee. Qualify and document the intended case-specific absolute/relative closure; do not globally tighten every solver iteration. Update the private Javadocs: they currently describe a gas/oil candidate, phase appearance/removal and a return requiring removal of one phase, which no longer describes this new branch. Add the documentation-impact assessment and a short update to the flash algorithm guide. Speed evidence and required gateThe normal new cost for an already balanced trace-water aqueous endpoint is a component-balance scan; it does not launch the cloned repair. Preserve that property. I ran three alternating base/head fresh-JVM pairs for complete repeated heptane/water flashes: Java 17.0.20, 256 MiB fixed heap, SerialGC,
SRK allocation was unchanged at about 65,000 bytes/flash; PR was about 64,648 → 64,568. Post-batch balance/fugacity diagnostics stayed equal at the recorded precision. These small shared-runner diagnostics establish neither universal slowdown nor universal speedup. CPA recovery was checked numerically; CPA timing was not qualified. Before declaring speed maintained, use paired full-flash and representative pump/process benchmarks covering dry gas, gas/oil, undersaturated trace water, established oil/aqueous, three phases, CPA and changed-state/recycle workloads. Report cold/warm/nearby-state time, allocation, recovery frequency and tail latency. Separate unchanged valid workloads from previously invalid outputs: an incorrect “fast” flash is not an acceptable performance baseline. Keep timing out of ordinary pass/fail unit tests. Relationship to other flash work
Important current dependency status: #4306 head Recommended implementation sequence
The numerical direction is consistent with Michelsen's stability/phase-split framework (Part I, Part II) and feasible-region safeguarded multiphase Rachford–Rice work (Okuno et al., 2010). Their inner-solver guarantees must not be mistaken for a global stability guarantee for the complete EOS flash. The goal is fewer invalid states and fewer recovery attempts, while keeping cheap, well-qualified paths cheap—not ever more unconditional stability calculations or looser thermodynamic tolerances. |
|
Implemented the review follow-up in 645e5e6. The original recovery remains narrowly scoped; ordinary TP/PS tolerances are unchanged. The repair now:
The final check is local numerical qualification, not a global stability certificate. Balanced endpoints incur no extra candidate clone or EOS initialization in the guard. Validation 130/130 focused tests pass, including public SRK/PR/CPA temperature sweeps, pressure changes and return, reused operations, cold-reference agreement, and actual pump entropy/mass/energy closure. Injected-state tests cover strict acceptance, failure reporting and the cheap guard. The original corrupted-endpoint regression remains. The same 130 tests pass in an isolated combined build with the targeted #4306 lifecycle fixes at Java 8-language compilation used ECJ; execution used Java 17 and verified baseline compiled dependencies. This was not a full local Maven/JDK build. Direct Spotless apply/check, documentation search, engineering inventory generation, diff checks, and both pre-commit/pre-push stages pass. The new GitHub build/test/Javadoc matrix remains authoritative. Performance evidence Baseline: this PR's previous exact head The complete-flash screen covers 12 families × unchanged/changed states, 5 batches × 32 measured flashes per state, 64 warmups (4096 for the three cheap families). All 720 batch-end numerical snapshots pass; all 360 matched baseline/candidate snapshots are identical. Median allocation differences are at most 4 bytes/flash. An injected SRK repair also produced exactly the same balance/fugacity residuals while reducing counted Short-warmup timing was noisy and showed increases in some CPA/condensate cases. Longer-warmup checks were therefore run on a fixed CPU with 1000 warmups, 5 measured batches, and three alternating JVM pairs:
The trace-water repeated-equilibrium checks used 1000 warmups and the same JVM flags, but were not CPU-pinned. Individual paired measurements still varied considerably; these are representative measurements, not a universal throughput or tail-latency guarantee. The longer-warmup checks did not reproduce the screen's CPA/condensate slowdown. The existing large-process harness also passed all 10 measured serial and 10 recycle changed-state samples on both versions (size 4, sequential strategy, 10 warmups). Single-pair median elapsed times were serial 117.0 → 92.2 ms and recycle 311.2 → 318.8 ms; these process timings are diagnostic only. Complete short-warmup screen (median paired changes; positive means slower)
This screen is retained including unfavorable measurements; it is not a steady-state speed gate. The expanded The algorithm guide and task log document the trigger, exclusions, acceptance contract, retry cost and validation limits. The original private process snapshot was unavailable during this follow-up; public cases qualify lifecycle behavior and do not establish full-plant convergence. Neither PR has been merged. |
|
Updated this PR in The CI failure was caused by subtractive cancellation in The performance changes retain the qualified gas/aqueous candidate instead of cloning/reflashing it, and stop non-finite or repeatedly stalled repair iterations early. The strict normalization ( Validation
Commands, run after the NeqSim Maven bootstrap: ./mvnw -B -ntp test '-Dtest=TPflash*Test,TPmultiflash*Test,PSFlash*Test,PHflash*Test,SystemThermo*Test,PumpTest,PumpSinglePhaseLiquidPSflashTest,ReservoirSurveillanceTest,WellFlowTest,FieldDevelopmentNPVTest,NorwegianOilFieldLifecycleTest,NamedComponentWaxFractionTest,WaxFractionSimTest,FieldNetworkTopologyTest,FieldWellNetworkProcessUnitTest,LoopedPipeNetworkTest' -DexcludedTestGroups= -Djacoco.skip=true
python3 devtools/run_spotless.py apply
python3 devtools/run_spotless.py check
python3 devtools/check_documentation_search.py
python3 devtools/build_engineering_coverage.py --check
pre-commit run --all-files --hook-stage pre-commit
pre-commit run --all-files --hook-stage pre-push
./mvnw -B -ntp javadoc:javadoc -DskipTestsPerformance and numerical evidence
The injected diagnostic explicitly sets the equilibrated methane/water gas/aqueous betas to 0.5/0.5 before timing the private repair (1,024 warmups, seven batches of 256 repairs; 42/42 measured endpoint snapshots pass). It measures the removed clone/reflash; it is not a complete public-flash speedup claim. The counted regression independently proves one candidate clone and one live initialization. The public TP harness covers 12 families × unchanged/changed conditions × 5 batches × 3 pairs, with 32 flashes/batch and 128 warmups (4,096 for the cheap cases). Maximum material residual is The initial short-warmup recycle result was noisy (+6.4% median, −1.0% to +26.0%); the longer-warmup check above resolves that uncertainty. Process iteration counts are unchanged, and the harness's existing mass/component/energy criteria all pass. This does not tighten the process recycle tolerance or claim private-plant convergence. Invalid old reservoir results are never counted as a fast baseline. Reproduce the public harnesses with each version's compiled classpath: java -Xms256m -Xmx256m -XX:+UseSerialGC -Xbatch -cp "$CP" neqsim.thermodynamicoperations.flashops.TPflashEndpointRecoveryBenchmark /tmp/flash.json 32 5 128
java -Xms256m -Xmx256m -XX:+UseSerialGC -Xbatch -cp "$CP" neqsim.process.processmodel.LargeProcessSteadyStateBenchmark serial changed 10 10 4 sequential /tmp/serial.json
java -Xms256m -Xmx256m -XX:+UseSerialGC -Xbatch -cp "$CP" neqsim.process.processmodel.LargeProcessSteadyStateBenchmark recycle changed 100 30 4 sequential /tmp/recycle.jsonCI: Exact-head run 38539212902. Model-specification jobs and Java 21 Javadocs have passed; full Ubuntu/Windows and slow-test jobs are still running. Formatting, pre-commit, documentation search, engineering coverage, CodeQL security and code-quality checks have passed. This remains an open PR; it has not been merged. |
Retain established arithmetic for ordinary and upward flow updates; rebuild only downward inventories whose difference update loses accuracy. Avoid scaling the VLP property basis to near-zero trial flow. Add component and volume closure evidence for the corrected reservoir production rate. Validation: 605 selected tests passed, 2 pre-existing disabled, no failures or errors; Java 8 language compilation and repository hygiene gates pass.
|
Published the CI regression repair at The previous head reproduced eight assertion failures and two errors. An independent exact-master run passed all 76 tests in those seven classes, confirming these were branch regressions. The common cause was applying positive inventory reconstruction to every flow update; ordinary floating-point endpoints and empty-stream behavior changed. The repair limits reconstruction to downward updates whose component or total difference update would lose more than The two-fluid VLP failure had a separate direct cause: scaling its property-only fluid clone to a near-zero trial rate. That scaling is unnecessary because the calculation reads intensive properties and already applies the trial rate to its velocities. Removing the setter call restores positive near-zero continuity and avoids one The reservoir reference change is supported by a material audit, not a widened tolerance. The old oil producer was 0.0322% above its requested rate (1,260,096.07887 versus 1,259,689.92248 kg/hr). Correcting that rate changes the ten-year pressure to 355.175066 bara. The existing 0.1 bar tolerance is retained, and the test now asserts all four stream rates, component inventory against integrated production/injection, and constant volume at every annual step. Independent maximum relative component imbalance was Validation on the published source tree
A focused reproduction command for the affected and inventory suites is: ./mvnw -Dtest=AdsorptionBedTest,DistillationColumnTest,MultiStreamHeatExchangerTest,SimpleReservoirTest,WellSystemTest,ConstraintActivityAnalyzerTest,ProcessModelOperatingActionTest,SystemThermoFlowRateInventoryTest -DexcludedTestGroups= -Djacoco.skip=true testPerformance limits: three fresh-JVM setter pairs and three warmed 10,000-iteration process pairs had substantial timing variability. The warmed process CPU differences ranged from −20.3% to +31.1%; allocation varied by less than 0.05%, and all 60,000 measured process iterations were valid. These measurements do not establish a speedup or a regression. The defensible optimization is removal of the two unnecessary VLP initialization calls; the cancellation guard adds no EOS initialization, clone or temporary array. The earlier trace-water candidate-transfer optimization is unchanged. Related clone/root-selector problems are being repaired independently in #4306; its changes are not copied into this branch. GitHub CI on this new exact head is pending. No merge performed. |
Recheck cached missing artifacts with -U and retry only Maven plugin or dependency resolution failures. Preserve failing exit codes and fail fast on compilation, missing-test, and numerical test failures. Validation: 11 shell behavior checks; plugin resolution probe; Spotless apply/check, documentation audit, pre-commit and pre-push all passed. Documentation impact: CI recovery policy recorded in workflow and task log.
Carry the bounded Maven-resolution retry into the renamed core workflow, retain all eight slow-test shards and four historical check contexts, and preserve both task-log entries. Keep all nine original Java source/test files byte-identical and regenerate engineering coverage. Validation: direct Spotless apply/check, documentation search, engineering coverage check, both pre-commit stages, workflow structure/Bash checks and whitespace checks passed. Isolated current-master+#4329+#4320 integration passed 165 tests across 21 classes with no failures/errors/skips on Java 17. Full exact-head GitHub CI remains pending.


Master integration — 2026-10-15
Integrated head
8d271e2f014b024628a9675d1de299bd9d792756incorporates master1798b61092aa356297402af02fff9d9e98749e5a, including the merged shared mixing and cold-wax fixes. All source changes combined cleanly; regenerated engineering coverage resolves the integration conflict. The combined tree is403496f3b981ddda11b7d6184a49e4077d255949.Validation: 41 focused tests passed, zero failures/errors/skips, covering named cold wax, trace-water recovery, flow-rate inventory, wells/reservoir and deterministic mixing accumulators. Stable direct formatting, both hook stages, documentation search, coverage generation/check and whitespace checks passed. Documentation impact: existing guides and task-log entries from both branches are retained; this integration introduces no new API or numerical tolerance changes. Earlier validation below applies to its recorded checkpoints. Publication and merge follow the user's explicit instruction to integrate in dependency order; final-head CI is tracked separately.
Coordination and merge gates — 2026-10-14
Reviewed published head:
0915f16e49e992d6c103618ad706d67fce20012e.All current-head checks now pass (38 successful check runs and one skip). The master integration retains all nine original Java files byte-for-byte and carries the bounded dependency-resolution retry in the canonical renamed workflow. The two prior inline review threads are resolved; comments are not approvals.
The actual patch spans thermodynamics (
TPflash,SystemThermo) and process simulation (WellSystem), so it requires an accepted NRC decision, covered by the proposed #4331 record, plus affected-owner and independent flash/process-domain review. Independent maintainer coverage must address observable failure and inventory contracts. These review roles need not mean three separate people; no applicable approval is currently recorded.Integration: After #4329 and before general #4306 integration. The isolated master+#4329+#4320 numerical trial passed 165 tests across 21 classes, with zero failures/errors/skips on JDK17; this does not replace final-head CI or performance qualification.
Original implementation and earlier validation history
Java 8 CI recovery — 2026-10-14
Updated at
402db73e7a99c18d849f192ae69224a22a01f162. VALIDATION PENDING CI on this exact head.The failed Java 8 model-specification job stopped before compilation: Maven could not resolve
maven-resources-plugin:3.3.1from Central. The missing evidence/summary errors followed because no tests ran. The published plugin supports Java 8 (Apache requirements).The model-specification workflow now uses
-Uto recheck missing artifacts and makes one additional attempt after a ten-second delay, only for Maven's terminalPluginResolutionExceptionorDependencyResolutionExceptiondiagnostics. Compilation errors, failed assertions, missing tests and fork failures remain fatal without retry. Persistent resolution failures preserve a nonzero exit status. Test selection and required evidence checks are unchanged. This is a CI-only change; no numerical code or calculation performance is affected.Validation:
./mvnw -B -U -ntp -f pomJava8.xml org.apache.maven.plugins:maven-resources-plugin:3.3.1:help: passed on local OpenJDK 17. This is plugin validation, not a Java 8 runtime test.python3 devtools/run_spotless.py apply,python3 devtools/run_spotless.py check,python3 devtools/check_documentation_search.py,pre-commit run --all-files --hook-stage pre-commit,pre-commit run --all-files --hook-stage pre-push, andgit diff --check: all exited 0 after the NeqSim Maven bootstrap.Latest CI regression repair — 2026-10-14
Updated at
15d1b8defe7cff39899ff8592f8ed75baebf5e2f. Local validation passed; GitHub CI is pending on this exact head. The ten failing cases from the previous head were reproduced; all 76 tests in the seven affected classes pass on exact masterb0de5165.The broad regression came from rebuilding component inventories on every flow change. This repair retains the established arithmetic for upward and same-rate changes, and for downward changes whose prospective component and total inventories remain accurate to
1e-12relative. It reconstructs positive normalized inventories only when a downward difference update loses that accuracy. The existing extreme inventory and trace-component protection remains covered.Two-fluid VLP now keeps its cloned reference inventory while reading intensive properties, applying the requested rate in the existing velocity calculation. This avoids the unnecessary
init(0)andinit(1)performed by the removed setter call and prevents a failed flash at positive near-zero trial rates. Tests cover near-zero continuity, reference-flow magnitude invariance and source preservation.The reservoir reference is updated with physical evidence, keeping the existing 0.1 bar tolerance: the old setter produced 1,260,096.07887 kg/hr oil for a requested 1,259,689.92248 kg/hr (0.0322% excess). The corrected rate gives 355.175066 bara after ten years. The test now checks all four requested stream rates, each component balance and constant volume at every annual step. An independent audit found maximum relative component imbalance
1.50e-16and relative volume error below1.5e-12.Validation
--release 8; Maven build and tests run on OpenJDK 17.0.20. This is not a Java 8 runtime test.This PR remains independent of #4306. Its separate clone/root-selection foundations are related work, not included here. Neither PR is merged by this update. No private-plant convergence or global stability claim is made.
Previous repair and qualification
Latest repair — 2026-10-14
Updated at
2b67ee03629bf845687d14e8a985be982fd4f8a0in response to the maintainer's request to fix this PR and preserve speed. VALIDATION PENDING CI on this new head. The original CI failure was reproduced and repaired.Root cause and changes
The public reservoir-to-producer path scales a large inventory down to a small flow.
SystemThermo.setTotalFlowRatesubtracted almost equal large component amounts, losing significant digits. A later TP flash then received overall fractions that did not sum to one. Twenty beta solves stagnated at a material residual of9.076572593524901e-8; changing PS tolerances cannot fix this feed.1e-12normalization /1e-10material /1e-8log-fugacity gates, including final failure reporting.1e-12–1e18mol, retaining a1e-10nitrogen fraction, and a counted gas/aqueous candidate-transfer regression.Verified validation
ReservoirSurveillanceTest#testHistoryRecordedreproduced the CI failure.b0de5165, with slow-test exclusions disabled. Includes every previously failing reservoir/well/field class, TP/PS/PH, SRK/PR/CPA, pumps, sour gas, sulfur phase extraction, flow units, inventory and lifecycle tests.--release 8; execution and the repository's Maven build used OpenJDK 17.0.20. The current repository POM targets Java 17. This does not claim a Java 8 runtime test.javadocexecutable.Three fresh-JVM pairs of an injected gas/aqueous recovery diagnostic show 39–53% less thread CPU time (median 48%) and 46% less allocation (71,184 → 38,544 bytes per recovery), with all 42 endpoint snapshots passing. This diagnostic times the explicitly induced private repair, not a complete public flash. Complete public-flash comparison: 720/720 valid snapshots, every paired state identical; family median CPU changes −4.0% to +4.6%, allocation unchanged. Changed-state process checks pass; after 100 warmups the recycle elapsed-time differences range from −1.93% to +0.88%. Full methods, timing limits and reproduction commands are recorded in the final validation comment. Incorrect old reservoir outputs are not counted as a speed baseline.
This fix remains independent of #4306. Its flow setter is not touched by that PR. Current master was merged to resolve generated-coverage conflicts, and its network/well-flow/wax changes were included in the expanded validation. #4306's separate blockers are not certified by this validation. Full private-plant convergence, experimental accuracy and global stability are not claimed.
Original implementation and earlier qualification
What was the bug?
For a fluid containing mostly oil and a little water, the TP flash could occasionally return the wrong amounts of oil and water. The resulting phase split did not conserve the original fluid composition.
An extremely small temperature change could therefore produce a large, artificial entropy jump. The pump's PS flash was trying to find a temperature with the required entropy, but this jump prevented convergence and caused a temperature-resolution error.
The existing trace-water recovery skipped states that already had an aqueous phase. Its refinement also expected a new phase to appear, so it could not repair the amounts of two phases that were already present.
What changed?
1e-10), normalization (1e-12) and fugacity (1e-8) checks, together with the existing candidate-selection checks. The 50/50 split is just a numerical starting point, not the final physical result.Validation
Review follow-up implementation (645e5e6)
solveBeta().IllegalStateExceptioninstead of returning silently. Ordinary TP/PS tolerances are unchanged.Follow-up validation
518867aapplied in an isolated combined build.