Fix drying condensed latent heat factor#124
Conversation
Mazhar331
left a comment
There was a problem hiding this comment.
Maintainer review — issue #24 condensed-phase latent-heat factor.
The change is correct and well-scoped. Drying.energy_balance removes the unexplained * 2 from the condensed-phase latent sink, matching the fix the issue triage requested. getHeatVaporization(..., basis='mass') already returns the full mass-basis latent heat [J/kg], and dry_rate is mass-basis [kg/m**3/s] after the #108 shared conversion, so their product is the full latent power [J/m**3/s] once. Dividing by denom_cond [J/m**3/K] yields [K/s]. The added Numpy-format docstring documents this units contract.
Verification performed (locally, conda run -n pharmapy, Python 3.10.20)
pytest tests/test_drying_latent_heat_factor.py tests/test_drying_energy_rate_basis.py -q— 2 passed.- Full lanes:
pytest tests/ -m assimulo— 15 passed, 40 deselected;pytest tests/ -m "not assimulo"— 40 passed, 15 deselected (55 total). - Recomputed the new fixture by hand:
dry_rate[:, [0,2]] * latent_heatsums to[256000, 338000, 128000] J/m**3/s,denom_cond = 900000 J/m**3/K, sodTcond_dt = [-256000, -338000, -128000]/900000 K/s. Matches the asserted literals, which are implementation-independent. - Red-on-base mutation: re-added
* 2to the source line in a scratch worktree and re-ran. Bothtest_drying_latent_heat_factor.pyandtest_drying_energy_rate_basis.pyfailed, each by exactly the 2× factor (observed values were exactly double the expected). Putting the bug back makes the tests fail, which confirms they guard the fix. The three node values are distinct, so a reorder/transpose would also be caught by value, and the volatile-count axis (2) differs from the node axis (3), avoiding a shape-collision ambiguity. - Grepped the repo: no other call site relied on the doubled value, and no other doubled-latent expression exists on this PR's head. (The
* 2still present in the working tree of the #81 branch is that separate PR's copy, consistent with the coordination note that whichever of #119/#124 merges second needs a small rebase.)
Blocking
None.
Nonblocking
- Docstring
Returnssection omits thereturn_terms=Trueoutput. See inline comment.
Linked issue
Closes #24 is correct: the PR fully resolves the issue's core ask (remove the unphysical factor of two from the condensed-phase evaporative-cooling sink).
Merge readiness
No blocking findings. This review is posted as a COMMENT because I authored this PR — this account cannot submit a formal APPROVE/REQUEST_CHANGES on its own PR; an eligible maintainer's approval is needed if branch protection requires one. Live GitHub state currently reports mergeStateStatus: CLEAN and an empty reviewDecision (no approval gate reported).
|
Addressed the PR #124 review feedback in commit Changes made:
Tests run:
Post-push CI on |
|
Verification pass (read-only) at head Comment-by-comment status
No blocking findings were raised in the review. Verification
Remaining issues: none. Another review round: not warranted. The follow-up is exactly the documentation clarification the comment requested — docstring-only, no behavior, public-API, or CI change. Merge readiness: |
| ``dry_rate`` gives the full latent power [J/m**3/s]. | ||
| """ | ||
|
|
||
| # temp_ref = 298 |
There was a problem hiding this comment.
Do we need this? Missing units regardless
There was a problem hiding this comment.
Addressed in 733f149: the inactive heat-loss expression was removed instead of keeping a commented formula, and the behavior-preserving placeholder is now explicit np.zeros_like(temp_gas) # [J/m**3/s].
There was a problem hiding this comment.
Follow-up in 55a530b: completed the stricter units/docs audit for this branch, normalized touched unit annotations to [...], and updated NumPy-style docs on touched functions. Validation: /home/mlaljee/miniforge3/envs/pharmapy/bin/python /tmp/pharmapy_unit_audit.py PharmaPy/Drying_Model.py tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -q -> 2 passed; CRLF-aware git diff --cached --check -> passed.
There was a problem hiding this comment.
Addressed at 7c12c1d: the inactive heat-loss formulas were removed, the retained placeholders are annotated as [J/m**3/s], and the drying-model unit audit now covers the flagged temperature, molecular weight, heat-capacity, permeability, and surface-area terms.
| @@ -476,7 +518,7 @@ def energy_balance(self, time, temp_gas, temp_sol, satur, y_gas, x_liq, | |||
| # heat_loss_cond = self.h_T_loss * self.a_V * (temp_sol - self.T_ambient) | |||
There was a problem hiding this comment.
What's the point of this comment? Should we define self.a_V elsewhere?
There was a problem hiding this comment.
Addressed in 733f149: the stale a_V comment and inactive heat-loss formulas were removed. a_V remains defined by the existing bed surface-area calculation; this PR does not introduce a new heat-loss model.
There was a problem hiding this comment.
Follow-up in 55a530b: completed the stricter units/docs audit for this branch, normalized touched unit annotations to [...], and updated NumPy-style docs on touched functions. Validation: /home/mlaljee/miniforge3/envs/pharmapy/bin/python /tmp/pharmapy_unit_audit.py PharmaPy/Drying_Model.py tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -q -> 2 passed; CRLF-aware git diff --cached --check -> passed.
There was a problem hiding this comment.
Addressed at 7c12c1d: the stale a_V inline comment was removed. a_V remains the existing bed surface-area calculation and is now documented as [m**2/m**3]; relocating it would be outside #24.
|
|
||
| # The mass-rate latent powers are [256000, 338000, 128000] J/m**3/s. Issue #24 | ||
| # separately tracks the existing factor of two applied to those powers. | ||
| # The mass-rate latent powers are [256000, 338000, 128000] J/m**3/s. |
There was a problem hiding this comment.
Units in []. Also, do we still need to talk about issue 24?
There was a problem hiding this comment.
Addressed in 733f149: the test comment now uses [...] units and no longer talks about issue 24; it states the latent powers directly as [J/m**3/s].
There was a problem hiding this comment.
Follow-up in 55a530b: completed the stricter units/docs audit for this branch, normalized touched unit annotations to [...], and updated NumPy-style docs on touched functions. Validation: /home/mlaljee/miniforge3/envs/pharmapy/bin/python /tmp/pharmapy_unit_audit.py PharmaPy/Drying_Model.py tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -q -> 2 passed; CRLF-aware git diff --cached --check -> passed.
There was a problem hiding this comment.
Addressed at 7c12c1d: the test comments now use [...] units and no longer refer to issue 24. The fixture uses unequal node/species counts and explicit expected values.
|
Pushed Addressed the new drying-model comment cleanup by removing stale script notes, old commented state/interpolation snippets, the obsolete The #24 latent-heat behavior is unchanged except for the intended fix: no extra factor of two, and Verification: unit audit passed; focused tests |
Read-only verification pass — head
|
| Thread | Comment | Status |
|---|---|---|
PRRT_kwDOLmbe4s6R3ub8 (resolved) |
Returns section omitted the return_terms=True output |
Addressed |
PRRT_kwDOLmbe4s6SZd8r |
@bernalde, Drying_Model.py L470: "Do we need this? Missing units regardless" |
Addressed |
PRRT_kwDOLmbe4s6SZeV4 |
@bernalde, Drying_Model.py L518: "What's the point of this comment? Should we define self.a_V elsewhere?" |
Partially addressed |
PRRT_kwDOLmbe4s6SZetT |
@bernalde, test_drying_energy_rate_basis.py L119: "Units in []. Also, do we still need to talk about issue 24?" |
Addressed |
No comment was declined. @bernalde's review body was empty, so the three inline comments are the full set.
Details:
- L470 — tracing the diff hunk, the targeted line is the stale
# temp_ref = 298. It is gone at head (grep temp_refreturns nothing), and the commented-outheat_loss/heat_loss_condformulas were removed too (Drying_Model.py:520,:532are nownp.zeros_like(...) # [J/m**3/s]). The reply in that thread describes a different line than the one commented on, but the outcome covers both readings. - L518 — the commented
# heat_loss_cond = self.h_T_loss * self.a_V * ...line is removed. Thea_Vhalf is answered only in the thread reply, not in code:Drying_Model.py:659(self.a_V = moments[2] * (1 - porosity) / moments[3]) still has no unit annotation, while the disabled alternative on:658keeps# [m**2/m**3].a_Vfeedsheat_transf(:517-519) andget_drying_rate(:240). Adding# [m**2/m**3]to:659would close this. - L119 — head reads
# The mass-rate latent powers are [256000, 338000, 128000] [J/m**3/s].Units bracketed, issue-High: [Drying] Condensed-phase energy balance doubles latent heat #24 narrative removed.
Test evidence
I re-ran the guard check against the current head rather than trusting the earlier log.
I put the bug back: in a throwaway copy of the head Drying_Model.py I re-added the * 2 to the condensed latent term (drying_terms = (dry_rate[:, self.idx_volatiles] * latent_heat * 2).sum(axis=1), Drying_Model.py:533-535) and ran the two head tests against that mutated copy. Both tests failed — 3/3 elements mismatched, observed values exactly 2× expected ([-0.568889, -0.751111, -0.284444] vs [-0.284444, -0.375556, -0.142222]). The tests failing here is the reassuring outcome: it proves they would have caught the original bug and still guard the fix today. The control run against the unmutated head source passed 2/2, so the failure is attributable to the mutation and not to the harness. I confirmed the mutated file — not the installed package — was the one imported.
Expected values are hand-computable and independent of the production expression: 0.036·2e6 + 0.184·1e6 = 256000, 0.054·2e6 + 0.230·1e6 = 338000, 0.018·2e6 + 0.092·1e6 = 128000 [J/m**3/s], over denom_cond = 1000·0.5·1000 + 0.5·0.5·2000·800 = 900000 [J/m**3/K]. The three node values are distinct, so a reorder or transpose would be caught by value, not merely by shape.
One fixture note: in tests/test_drying_latent_heat_factor.py the node count (3) equals the species count (3) for y_gas and dry_rate. The sibling test deliberately avoids that collision for latent_heat (test_drying_energy_rate_basis.py:64-66). Benign here because the values are asymmetric, but it drops a convention the neighbouring file documents.
CI
Run 29867685160, head_sha confirmed 55a530be6826390738e7b2ffd2f292645e52a14b:
- Core tests — SUCCESS (40 passed, 5 skipped, 6 deselected)
- Assimulo integration tests — SUCCESS (15 passed, 40 deselected)
Both drying tests appear PASSED in the Assimulo job log. No runs were started, re-run, or cancelled.
Units / documentation / comments audit
The changed hunks and the surrounding call path, not only the fixed line.
1. dTg_dt is annotated [K/s], but the PR's own annotations say it is not. Drying_Model.py:526 labels conv_term [J*kg/m**6/s] and :502 labels denom_gas [J/m**3/K]; their quotient is [kg*K/m**3/s]. Line :528 also adds conv_term to sensible_heat [J/m**3/s]. The Notes block (:482-489) acknowledges this as dimensional debt, yet :528, the Returns section (:472-474), and unit_model:350 all still assert [K/s] unqualified. The extra factor is rho_gas, entering twice (via dTg_dz at :524 and explicitly at :526). Please either qualify those three labels or drop the Notes claim — as written the file contradicts itself.
2. mw_avg_gas is labelled [g/mol] but is not a molar mass. :492 (and :316) compute np.dot(y_gas, self.Vapor_1.mw). y_gas is documented as mass fractions (:454-455) and passed as mass_frac= to getCp/getViscosity. On a mass basis the mixture molar mass is 1 / Σ(w_i / M_i), not a mass-weighted arithmetic mean. The label propagates into rho_gas (:320) and cvg_mix (:499), and denom_gas sits in the reviewed balance. Pre-existing, but this PR is what converts an unlabeled ambiguity into a definite — and incorrect — unit claim.
3. Saturation denominator asymmetry between the two balances. material_balance floors the void fraction (:415-416, epsilon_gas[epsilon_gas <= eps] = eps); energy_balance does not (:501), so denom_gas (:502) goes to zero as satur → 1 and dTg_dt diverges. Both lines are inside the touched region.
4. self.drying diagnostic uses the wrong denominator, and the docstring mislabels it. :543 is drying_terms / denom_gas — condensed-phase latent power over the gas capacity. It is [K/s] dimensionally, but it matches no term in either balance: dTg_dt (:528) uses sensible_heat, and dTcond_dt (:539) divides by denom_cond. Returns at :479-480 describes the remaining tuple entries as "gas-temperature-rate contributions", which is not true for this one.
5. The states_di metadata change is user-visible and diverges from the repo convention. :168-179 changes 'units': '' → '[-]' and 'K' → '[K]'. These are not comments: Plotting.py:184-186 and :267-271 append any non-empty value to the axis label as ylabel + ' (' + latexify_name(units, units=True) + ')'. Drying plots will now read temp_gas ([K]), and saturation / y_gas / x_liq axes gain a ([-]) suffix that the previous empty string suppressed. No other PharmaPy module brackets these values (18 × 'K', 10 × '', none bracketed). The review request was "Units in []" on a test comment; extending it to plotting metadata changes rendered output, is untested, and is inconsistent with the rest of the package. Suggest reverting these five entries.
6. Constants left unannotated inside annotated hunks. :69 self.T_ambient = 298 (no [K]) and :75 self.h_T_loss = 30 (no [W/m**2/K]) sit immediately below :72/:74, which this diff did bracket.
7. Adjacent call-path quantities feeding the reviewed balances are unannotated. In solve_unit: :634 alpha [m/kg], :635 rho_sol [kg/m**3], :636 porosity [-], :648 k_perm, :652 cp_sol [J/kg/K], :659 a_V [m**2/m**3], :671 dPg_dz [Pa/m], :672 pres_gas [Pa] — all consumed by denom_cond, heat_transf, or vel_gas.
8. Comment regression introduced by this PR. sat_red (:305-306) and k_ra (:307) are computed and never used. Their only consumer and explanation was the commented # vel_gas = self.k_perm * k_ra * self.dPg_dz / visc_gas line, which this PR deleted. Three lines of dead arithmetic now carry a # [-] label and no reason to exist — either remove them or restore a one-line note.
9. Blanket multi-unit labels. :290-291 states_reord … # [-] and [K] and :349 inputs … # inlet states: [-] and [K] annotate a heterogeneous array with two units at once instead of per quantity. :297 x_liq[:, -2] = 0 # [-] has a unit but no rationale for the magic index.
10. Missing docstring on the reviewed call path. get_y_equilib (:201) has none, despite converting mass fractions to mole fractions and returning the equilibrium gas mole fractions [-] that set the drying rate consumed by both balances.
11. Scope. +149/−98 in Drying_Model.py for a one-line physics fix. Beyond the annotations, the diff removes a # TODO: check term by term in material balance down this line marker and a dead drying_terms = rho_gas / self.rho_liq * cpg_mix assignment. Both are defensible; neither was requested, and the TODO is discarded rather than addressed.
The #24 fix itself — drying_terms = (dry_rate[:, self.idx_volatiles] * latent_heat).sum(axis=1) at Drying_Model.py:533-535 — is correct and dimensionally sound: [kg/m**3/s] × [J/kg] = [J/m**3/s], divided by denom_cond [J/m**3/K] gives [K/s].
Remaining issues
Items 1–5 above should be resolved before merge. Item 5 changes rendered output and was not requested by any reviewer; items 1, 2 and 4 are inaccurate claims in newly added documentation on the reviewed function. Items 6, 7 and the a_V annotation at :659 are one-line fixes that also close the outstanding half of the L518 comment.
Gate
master is unprotected (GET /branches/master/protection → 404), so no approving-review count is enforced. mergeStateStatus is CLEAN, but reviewDecision is CHANGES_REQUESTED: @bernalde, that verdict holds until you re-review or dismiss it — neither the author nor CI can clear it.
| temp_cond_init = self.CakePhase.Solid_1.temp # [K] | ||
| temp_gas_init = self.Vapor_1.temp # [K] | ||
| z_cake = self.CakePhase.z_external # [m], for drying_script_inyoung | ||
| # z_cake = self.CakePhase.z_external # This line for 2MSMPR_Filter.py |
There was a problem hiding this comment.
Addressed in 6337303: removed the stale for drying_script_inyoung note and the old 2MSMPR_Filter.py z_cake comment. The active assignment now keeps only z_cake = ... # [m].
| porosity = self.CakePhase.porosity # [-] | ||
|
|
||
| xliq = states_prev[:, num_comp + 1: num_comp + 1 + self.num_volatiles] | ||
| # xliq = states_init[:, num_comp + 1: num_comp + 1 + self.num_volatiles] |
There was a problem hiding this comment.
This comment might be removed, please verify
There was a problem hiding this comment.
Addressed in 6337303: removed the # Physical properties banner and adjacent stale commented initialization block. The active quantities now either carry bracketed units or are covered by the solve_unit NumPy notes.
|
|
||
| self.k_perm = 1 / alpha / rho_sol / (1 - porosity) | ||
| self.k_perm = 1 / alpha / rho_sol / (1 - porosity) # [m**2] | ||
| # self.rho_liq = rho_liq |
There was a problem hiding this comment.
Addressed in 6337303: removed the obsolete commented self.rho_liq assignment. The solve_unit notes now state that the initial rho_liq [kg/m**3] is only used for the irreducible-saturation estimate, while unit_model recomputes self.rho_liq from the current liquid state during integration.
# Conflicts: # PharmaPy/Drying_Model.py # tests/test_drying_energy_rate_basis.py
Summary
Drying.energy_balance.energy_balanceunits contract now that Critical: [Drying] Liquid saturation balance mixes molar drying rate with mass density #20/Critical: [Drying] Gas species balance adds molar source terms to mass-fraction states #21/High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48 have landed onmaster:dry_rateenters on a mass basis andlatent_heatis mass-basis, so their product is already the full latent power.Closes #24.
Refs #20, #21, #48, #103, #108, #116, #119.
Acceptance Criteria
dry_rate[:, idx_volatiles] * latent_heatonce.material_balanceandenergy_balance.Dimensional Analysis
unit_modelconvertsget_drying_rateoutput from[mol/m**3/s]todry_rate[kg/m**3/s]using molecular weights[kg/mol].getHeatVaporization(..., basis='mass')returnslatent_heat[J/kg].(kg/m**3/s) * (J/kg) = J/m**3/s.denom_condis a volumetric heat capacity[J/m**3/K], so-drying_terms / denom_condis[K/s].Coordination Notes
master.masterafter Fix drying gas mass-rate balance #108 (c7a38a4), so it preserves the Fix drying gas mass-rate balance #108 shared mass-basis conversion and does not revive PR Fix drying saturation mass-rate conversion #103's local conversion.* 2unchanged for High: [Drying] Condensed-phase energy balance doubles latent heat #24; this PR updates that pinned expectation and adds a direct latent-power regression.PharmaPy/Drying_Model.pyandtests/test_drying_energy_rate_basis.pyfor issue High: [Drying] Gas balance and Darcy velocity produce order-of-magnitude drying transient errors #81. It does not change the same latent-heat expression, but whichever branch merges second may need a small rebase/context resolution.Tests
conda run -n pharmapy python -m pytest tests/test_drying_latent_heat_factor.py -qfailed by 2x in the condensed temperature-rate assertion.conda run -n pharmapy python -m pytest tests/test_drying_latent_heat_factor.py tests/test_drying_energy_rate_basis.py -q- 2 passed.conda run -n pharmapy python -m pytest tests/test_drying_model.py tests/test_drying_energy_rate_basis.py tests/test_drying_latent_heat_factor.py -q- 6 passed.conda run -n pharmapy python -m pytest tests/ -m "not assimulo" -q- 40 passed, 15 deselected.conda run -n pharmapy python -m pytest tests/ -m assimulo -q- 15 passed, 40 deselected.conda run -n pharmapy python -m pytest --collect-only -q- 55 tests collected.git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --check- passed.Environment:
conda run -n pharmapyusing Python 3.10.20.Branch Hygiene
master.origin/masteratc7a38a412d1ab26c8d91ef975ff4a5b3a7393c3b.PharmaPy/Drying_Model.pyandtests/test_drying_energy_rate_basis.pyfor High: [Drying] Gas balance and Darcy velocity produce order-of-magnitude drying transient errors #81, but this PR changes the High: [Drying] Condensed-phase energy balance doubles latent heat #24 latent-heat expression and expectation.Review Follow-Up Metadata
6337303directly to existing branchfix/issue-24-drying-latent-heat.origin/masterat18442e9into the branch, preserving the High: [Drying] Gas balance and Darcy velocity produce order-of-magnitude drying transient errors #81 relative-permeability Darcy gas-velocity fix and the High: [Drying] Condensed-phase energy balance doubles latent heat #24 condensed latent-heat fix.z_cakescript notes, old commented state/interpolation lines, the# Physical propertiesbanner, the obsoleteself.rho_liqcomment, unusedsauter_diam, old gas-pressure notes, and stale drying-rate/p_gas_total alternatives.z_cake[m],x_csd[m],csd[-],mom_zero[-],y_volat[-],dry_volatiles[mol/m**3/s],dry_rates[mol/m**3/s],mw_avg_gas[g/mol],temp_gas_inputs[K],cpg_mix[J/kg/K], anddenom_cond[J/m**3/K].rho_liq[kg/m**3]is used only for irreducible saturation and whyunit_modelrecomputesself.rho_liqduring integration.11 passed, 2 warnings; full suite71 passed, 2 warnings; CRLF-aware diff check passed.