Fix VaporPhase mass-basis latent heat weighting - #100
Conversation
andres9403
left a comment
There was a problem hiding this comment.
Maintainer review — PR #100 (VaporPhase mass-basis latent heat weighting)
Reviewed at head 56c6835. The change correctly resolves issue #18: in VaporPhase.getEnthalpy, the per-species J/kg latent-heat term is now weighted by mass fractions when basis='mass' and by mole fractions otherwise, instead of using mole_frac unconditionally.
What I verified
- Fix matches the diagnosis.
PharmaPy/Phases.py:732-733selectsfrac = mass_frac if basis == 'mass' else mole_frac, exactly the fix recommended in the issue audit and the independent Codex verification.mass_frac/mole_fracare both resolved earlier (lines 687-693), so the mass branch is always defined. - Mole-basis path unchanged. The
elsebranch still usesmole_frac; thetotal_h=Falseper-species path (hVap = hSens + deltaVap) is untouched. All existing callers inEvaporators.py/Extractors.pypassbasis='mole'explicitly and are unaffected. - Regression test is meaningful.
tests/test_phases_vapor_enthalpy.pyuses two species of disparate MW (18 vs 100), equimolar, and independently recomputes the expected mass- and mole-basis mixture enthalpies plus the basis-invariance identityh_mass == h_mole * 1000 / mw_av, and checks the per-species path for both bases. I confirmed it is genuinely red on the pre-fix code shape (1.51e6vs expected9.46e5J/kg) and green after the fix — it locks in the behavior rather than passing vacuously. - Docstring cleanup fixes the
he default is Truetypo and documents thebasisparameter, consistent with the signature default ('mass').
Tests run locally (Python 3.10, numpy 1.26.4)
pytest tests/test_phases_vapor_enthalpy.py→ 1 passedpytest tests/ -m "not assimulo"→ 28 passed, 6 deselected- Temporary revert of the one-line fix → target test fails as expected, then restored
- Working tree clean; no stray artifacts. Assimulo-marked tests not run (Assimulo not installed locally; CI runs that marker group).
Findings
Nonblocking — pre-existing multi-temperature mass-basis shape mismatch (out of scope).
Not introduced by this PR, but adjacent to the changed line: in getHeatVaporization the basis='mass' branch reindexes deltahvap to only the subcritical columns (deltahvap[:, idx] / self.mw[idx]), reducing its second dimension from num_comp to len(idx). When any species is supercritical, the subsequent np.dot(deltaVap, frac) in getEnthalpy would then mismatch the full-length frac vector. The tests here keep all species subcritical (350 K), so this path isn't exercised. Worth a follow-up issue; no change requested in this PR.
Merge-readiness
- No blocking findings. Fix is correct, minimal, and covered by a regression test that fails without it.
mergeStateStatus: CLEAN.- This review is a COMMENT because I am the PR author — GitHub does not accept an
APPROVEfrom the author account. If branch protection requires a formal approval, another eligible maintainer will need to provide it; live state does not currently report aREVIEW_REQUIREDgate.
|
Addressed the review pass for PR #100. Commits pushed:
Main changes made:
Verification:
Comments intentionally not addressed in this PR:
Remaining gates:
|
Review-resolution verification — no further review round neededVerified at head
Another review round is NOT justified — the only finding was nonblocking and deferred by the reviewer, and no behavior changed beyond the commented lines. No further agent review needed. Remaining gate (governance, not a review comment): |
Summary
VaporPhase.getEnthalpy(basis="mass")so the per-species J/kg latent heat term is weighted by mass fractions.total_h=Falseper-species return path.Closes #18
Tests
pytest tests/test_phases_vapor_enthalpy.pypytest tests/ -m "not assimulo"pytest tests/ --collect-only -qgit -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --checkAssimulo-marked integration tests were not run locally because Assimulo is not installed in this environment; the CI workflow runs that marker group separately.
Branch Hygiene
masterorigin/masterat1c3a046fceb5c4da8b5fdb6e8b254f657d6e3cc7origin/masterincludesupstream/masterat400e1e13ad4ecd3ba202d5fae8c70ec73bdf9c3efix-issue18-latent-heat-weightingexists at an ancestor of currentorigin/masterand has no PR; this PR uses a fresh focused branch.