Fix drying gas mass-rate balance

[Mazhar331]

Summary

• Convert drying rates from molar to mass basis once in unit_model after get_drying_rate.
• Reuse that mass-basis rate in the liquid saturation/composition balance and gas species transfer term.
• Add drying model regressions for the conversion, saturation balance, and gas species source term.
• Document the molar/mass-basis contract and inline material-balance units.

Closes #21.
Refs #17, #20, #24, #48, #103.

Unit Contract

• get_drying_rate returns component volumetric drying rates on a molar basis [mol/m**3/s].
• unit_model converts that rate once using molecular weights [kg/mol]; after _drying_rate_mass_basis, self.dry_rate is [kg/m**3/s].
• material_balance consumes mass-basis drying rates [kg/m**3/s]; saturation, gas mass fractions, liquid mass fractions, porosity, and gas holdup terms are dimensionless [-].
• The material-balance derivatives remain dsat_dt, dygas_dt, and dxliq_dt in [1/s].
• energy_balance now receives the same mass-basis self.dry_rate, which is the shared conversion needed by High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48, but High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48 still needs energy-specific validation before it can close.

Coordination Notes

• Critical: [Drying] Gas species balance adds molar source terms to mass-fraction states #21 is part of the [Epic] Verified model and numerical correctness defects (#18-#66) #17 model-correctness audit.
• Critical: [Drying] Liquid saturation balance mixes molar drying rate with mass density #20 was closed by PR Fix drying saturation mass-rate conversion #103 with a local dry_rate_mass conversion inside material_balance. This PR intentionally removes that local conversion and replaces it with one shared conversion point in unit_model, so the Critical: [Drying] Liquid saturation balance mixes molar drying rate with mass density #20 saturation fix is preserved without a double MW/1000 conversion.
• get_drying_rate remains the molar volumetric-rate calculator (mol m^-3 s^-1), matching k_y * a_V * delta_y. This PR converts its output once before the model balances consume it.
• High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48 has the same molar-vs-mass root cause in energy_balance. Because energy_balance receives the converted self.dry_rate, this PR covers the shared MW/1000 conversion needed by High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48. It does not close High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48 because that issue still needs energy-specific validation.
• High: [Drying] Condensed-phase energy balance doubles latent heat #24 is separate from the mass-basis conversion: it tracks the unrelated latent-heat * 2 factor in the same energy expression. High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48/High: [Drying] Condensed-phase energy balance doubles latent heat #24 should be resolved with a clear follow-up diff, likely by removing the * 2 and adding an isolated energy-balance regression.
• This PR now documents the units contract for get_drying_rate, unit_model, and material_balance; final energy-balance unit validation remains pending High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48/High: [Drying] Condensed-phase energy balance doubles latent heat #24.

Tests

• conda run -n pharmapy python -m pytest tests/test_drying_model.py -q
• conda run -n pharmapy python -m pytest tests/ -m "not assimulo" -q
• conda run -n pharmapy python -m pytest tests/ -q
• git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --check

Branch Hygiene

• Base branch: master
• Source branch point: origin/master
• Stacked status: not stacked
• Prerequisites: none

[Mazhar331]

Maintainer review

The physics of this change is right. Consolidating the molar→mass conversion into a single point in unit_model is the correct shape of fix for #21: Liquid_1.name_species is the full species list, so mw aligns column-wise with dry_rate, the non-volatile columns are zero and therefore unaffected, mw is g/mol (consistent with mw_avg_gas / 1000 at Drying_Model.py:253), and the /1000 yields kg/mol. After the change transfer_gas is [kg m^-3 s^-1] / [-] / [kg m^-3] = [1/s], dimensionally consistent with the convection term, which is exactly what #21 asks for. Removing the local dry_rate_mass block from material_balance correctly avoids double-converting the #20 saturation fix.

What I cannot sign off on is the test coverage. The three new tests do not detect the bug this PR fixes. I verified this by mutation, not by reading:

1. Deleting the fix leaves the suite green. I removed self.dry_rate = self._drying_rate_mass_basis(self.dry_rate) (Drying_Model.py:274) — the entire fix — and ran pytest tests/test_drying_model.py: 3 passed. Both material_balance tests are fed dry_rate directly as a parameter, and _drying_rate_mass_basis is exercised in isolation, so nothing asserts that unit_model actually applies the conversion before the balances consume it.

2. The gas-species test passes when fed molar rates. I reverted the dry_rate fixture in test_material_balance_uses_mass_drying_rate_for_gas_species to the pre-fix molar values ([[2.0, 0.0, 4.0], [3.0, 0.0, 5.0]]) — the precise input shape bug #21 describes — and the test still passed. It cannot fail on a units error, because expected_dygas_dt re-derives the production expression from the same dry_rate input. The test does not set dryer.Liquid_1 at all, which is itself the tell: material_balance no longer touches MW, so a test living there cannot assert anything about mass basis.

Details and fixes are in the inline comments.

Blocking

The unit_model conversion has no regression coverage

Inline at PharmaPy/Drying_Model.py:274. The PR's stated regression is unguarded: a future refactor can drop the conversion line and CI stays green. Add a test that drives unit_model (or asserts on the dry_rate handed to the balances, e.g. by monkeypatching material_balance/energy_balance to capture it) and pins the mass-basis value.

test_material_balance_uses_mass_drying_rate_for_gas_species is tautological

Inline at tests/test_drying_model.py:120. expected_dygas_dt duplicates the implementation under test, so it asserts an identity rather than a behavior, and the name overstates what it checks. Replace the recomputation with hand-computed literals, as test_material_balance_uses_mass_drying_rate_for_saturation correctly does with mass_rate = np.array([0.22, 0.284]).

Nonblocking

energy_balance behavior changes with no test

Because energy_balance receives self.dry_rate, both sensible_heat (Drying_Model.py:368) and drying_terms (Drying_Model.py:397) are now scaled by mw/1000 — roughly 0.018 for water, 0.046 for the third species in the fixtures. dTg_dt and dTcond_dt therefore change by more than an order of magnitude. This is the correct direction (it is #48's root cause, as the PR body says), and I agree it should not close #48, but no test in the repository calls energy_balance or unit_model, so this is a large, entirely uncovered numerical change riding along in a PR titled for the gas balance. A unit_model-level test that asserts on the returned model_eqns would cover this and the blocking item above together. Note this also interacts with the * 2 latent-heat factor tracked in #24.

Mixed line endings introduced

Inline at PharmaPy/Drying_Model.py:203. Drying_Model.py is a CRLF file (697 CRLF lines); this PR writes LF-terminated lines into the hunks it touches, taking LF-only lines from 16 to 27. About 16 of the 33 +/- lines in the diff are pure end-of-line churn — def get_drying_rate(...) and return dry_rates show as modified while being byte-identical in content. git diff --ignore-cr-at-eol reduces the file's diff to 9 insertions / 6 deletions, which is the real change. There is no .gitattributes; either add one with *.py text eol=lf and normalize deliberately in a separate commit, or keep this diff CRLF-consistent so review and git blame stay clean.

self.dry_rate units are no longer documented

Inline at PharmaPy/Drying_Model.py:220. The removed comment in material_balance was the only place stating the basis of the rate. get_drying_rate remains molar while self.dry_rate is now mass — a real trap for the next maintainer. A one-line unit annotation on _drying_rate_mass_basis and at the unit_model assignment costs nothing.

Questions

None.

Tests run

All in the repository's pharmapy conda environment, at head 4029b17.

• pytest tests/test_drying_model.py -q -rs — 3 passed, 0 skipped.
• pytest tests/ -q -rs — 43 passed, 0 skipped, 2 warnings (both pre-existing: numpy.matlib PendingDeprecationWarning from Drying_Model.py:8, and a zero-mass/moles/vol RuntimeWarning from Phases.py:203).
• pytest tests/ --collect-only -m "" — 43 collected, identical to the default run, so no marker group is being silently deselected. The only registered marker is assimulo, and Assimulo is installed here.
• Mutation check A: deleted Drying_Model.py:274 in a scratch worktree → 3 passed (should have failed).
• Mutation check B: molar dry_rate fixture in the gas-species test → 1 passed (should have failed).
• Verified dry_rate / get_drying_rate have no consumers outside Drying_Model.py, so self.dry_rate switching basis leaks into no reporting or plotting path.
• Cleaned the __pycache__ directories my test runs created; note they are not covered by .gitignore in this repo, which is pre-existing and out of scope here.

Linked issue

Closes #21 is accurate. The gas transfer term at Drying_Model.py:329 now adds a mass source to a mass-fraction state, which is the core ask, and the two independent verifications on the issue both name MW / 1000 as the fix. The coordination notes on #20/#48/#24 match what the code actually does.

Merge readiness

Checks on the reviewed head 4029b17 are green: Core tests SUCCESS, Assimulo integration tests SUCCESS. mergeStateStatus is CLEAN, the head is level with master (0 commits behind), so PR-head and merge-result checks coincide here. master has no branch protection and GitHub reports no reviewDecision gate.

This is a COMMENT review because I am the PR author; GitHub rejects APPROVE and REQUEST_CHANGES from the author, so this account cannot provide a formal approval if one is wanted. No approval gate is currently reported by GitHub.

I would not merge this until the blocking issues above are addressed. Both are test-only changes and neither requires touching Drying_Model.py logic, which I believe is correct as written.

[Mazhar331]

Blocking — this line is the entire fix, and nothing tests it.

I deleted exactly this line in a scratch worktree and ran pytest tests/test_drying_model.py: 3 passed. Both material_balance tests receive dry_rate as a parameter, and _drying_rate_mass_basis is tested in isolation, so no test asserts that unit_model converts before material_balance and energy_balance consume the rate. A future refactor can drop this line and CI stays green — the regression test does not guard the regression.

Concrete fix: add a test that drives unit_model and pins the mass-basis rate actually passed downstream. Capturing it is cheap, e.g.

captured = {}
monkeypatch.setattr(dryer, 'material_balance',
                    lambda *a, **k: captured.setdefault('dry_rate', a[8]) and [np.zeros(2)]*3)

or assert directly on the returned model_eqns, which would also cover the untested energy_balance change (see the energy-balance-uncovered finding in the review body).

Separately, please annotate the basis here — after this line self.dry_rate is [kg m^-3 s^-1], while get_drying_rate still returns [mol m^-3 s^-1].

[Mazhar331]

Addressed in a2c80e1: added a unit_model test that captures the dry_rate passed into both balances and pins the mass-basis values, plus unit comments at the conversion site.

[Mazhar331]

Still addressed at current head. The unit_model handoff test continues to verify that the converted mass-basis dry_rate reaches both balance methods; 70b5ba7 only adds the requested unit/doc clarity around that path.

[Mazhar331]

Nonblocking — the comment this PR removed from material_balance was the only place documenting the rate's basis. Now get_drying_rate returns molar and self.dry_rate holds mass, with nothing saying so at either site.

A docstring or one-line annotation is enough:

def _drying_rate_mass_basis(self, dry_rate):
    """Convert molar drying rates [mol m^-3 s^-1] to mass rates [kg m^-3 s^-1]."""
    mw = np.asarray(self.Liquid_1.mw) / 1000  # [kg/mol]

    return dry_rate * mw

For what it is worth I checked the alignment and it is correct: self.name_species = self.Liquid_1.name_species (Drying_Model.py:129), so mw is indexed over the full species set and broadcasts column-wise against dry_rate; the non-volatile/supercritical columns are zero in get_drying_rate, so scaling them is a no-op.

[Mazhar331]

Addressed in a2c80e1: _drying_rate_mass_basis now has a unit docstring, and unit_model annotates the molar output from get_drying_rate and the mass-basis rate consumed by the balances.

[Mazhar331]

Addressed at current head. get_drying_rate is now documented as returning molar rates [mol/m**3/s], unit_model documents the single conversion point, and material_balance documents the mass-basis contract [kg/m**3/s].

[Mazhar331]

Nonblocking — this line is byte-identical in content to master; it appears in the diff only because its line ending changed from CRLF to LF. Drying_Model.py is a CRLF file (697 CRLF lines), and this PR writes LF lines into the hunks it touches, taking LF-only lines from 16 to 27.

About 16 of the diff's 33 +/- lines are pure EOL churn — return dry_rates below has the same problem. git diff --ignore-cr-at-eol collapses this file to 9 insertions / 6 deletions, which is the real change.

There is no .gitattributes in the repo. Either normalize deliberately (add *.py text eol=lf and convert in a separate commit) or keep this diff CRLF-consistent, so review and git blame stay readable.

[Mazhar331]

Addressed in a2c80e1: restored Drying_Model.py to CRLF-consistent line endings. The remaining model diff is visible with git diff --ignore-cr-at-eol.

[Mazhar331]

Still addressed at current head. Drying_Model.py remains CRLF in the worktree/index, and the CRLF-aware staged whitespace check passed.

[Mazhar331]

Addressed the review feedback in commit a2c80e1.

Changes made:

• Added test_unit_model_converts_drying_rate_before_balance_equations, which drives unit_model and verifies the converted mass-basis dry_rate reaches both material_balance and energy_balance.
• Replaced the gas-species expected value recomputation with hand-pinned literals.
• Added unit-basis documentation around _drying_rate_mass_basis and the unit_model conversion.
• Restored PharmaPy/Drying_Model.py to CRLF-consistent line endings; git diff --ignore-cr-at-eol now shows the real model diff as only the unit annotations.

Tests run:

• conda run -n pharmapy python -m pytest tests/test_drying_model.py -q — 4 passed.
• conda run -n pharmapy python -m pytest tests/ -m "not assimulo" — 33 passed, 11 deselected.
• conda run -n pharmapy python -m pytest tests/ — 44 passed.

CI on head a2c80e1:

• Core tests: pass.
• Assimulo integration tests: pass.

Not intentionally declined: none. Remaining follow-up is the already documented #48/#24 energy-specific validation, outside this PR's #21 scope.

[Mazhar331]

Verification of a2c80e1 against the review on 4029b17

Read-only pass. I re-ran the same two mutation checks that exposed the original gaps, plus an independent derivation of the new expected values. Both blocking findings are genuinely fixed — the tests now fail when the bug is reintroduced, which is the property that was missing before.

Blocking

unit-model-conversion-untested — Addressed. I deleted self.dry_rate = self._drying_rate_mass_basis(self.dry_rate) from unit_model in a scratch copy of head and ran the suite:

FAILED tests/test_drying_model.py::test_unit_model_converts_drying_rate_before_balance_equations
1 failed, 3 passed

Previously this mutation gave 3 passed. test_unit_model_converts_drying_rate_before_balance_equations captures the ninth positional argument handed to both balances and pins it to the mass-basis values, so the conversion is now load-bearing for CI. The exact-equality assertion is sound rather than incidental: eta_fun defaults to lambda sat, mass_frac: np.ones_like(sat) (Drying_Model.py:82) and mass_eta defaults to False, so limiter_factor is exactly 1 for this fixture.

gas-species-test-tautological — Addressed. The recomputed expression is gone, replaced by literals. I fed the test the pre-fix molar rates [[2.0, 0.0, 4.0], [3.0, 0.0, 5.0]]:

FAILED tests/test_drying_model.py::test_material_balance_uses_mass_drying_rate_for_gas_species
1 failed, 3 deselected

Previously that same mutation passed. I also re-derived the literals independently from the physics — dy_i/dt = mdot_i/(eps_gas*rho_gas*(1-S)) + y_i/(1-S)*dS/dt — in exact rational arithmetic, without reference to the PharmaPy source:

node0: [0.239912, -0.000704, 1.2265786666666667]
node1: [0.2877728, -0.0007952, 1.2265530666666666]

These match the committed values, so the literals encode the intended physics rather than a paste of whatever the implementation happened to print.

Nonblocking

dry-rate-units-comment — Addressed. _drying_rate_mass_basis now carries a units docstring, and unit_model annotates both the molar output of get_drying_rate and the mass-basis rate consumed by the balances. The molar/mass boundary is now stated at the point where it matters.

mixed-line-endings — Addressed. Drying_Model.py at head is 726 CRLF lines and 0 LF-only lines. The churn is gone and the diff now shows only real changes.

energy-balance-uncovered — Partially addressed. The new test asserts that energy_balance receives the mass-basis rate, which pins the contract and is the regression guard that matters most. But energy_balance is itself monkeypatched to a stub, so its numerics — sensible_heat (Drying_Model.py:368) and drying_terms (Drying_Model.py:397) — are still not exercised by any test; grep -rn energy_balance tests/ finds only the stub. Consequently np.testing.assert_allclose(model_eqns, np.zeros(states.size)) is vacuous: it holds because both balances are stubbed to zeros, not because of anything the model computed.

This is not worth blocking on. It was Nonblocking to begin with, the coverage gap is pre-existing rather than introduced here, and the PR body already defers energy-specific validation to #48/#24, where the * 2 latent-heat factor has to be settled anyway. Folding a real energy_balance assertion into that follow-up is the cleaner sequencing.

Evidence

Everything below ran against PR-head content (a2c80e1) in a scratch copy; the checkout was not modified and no CI was started.

• pytest tests/test_drying_model.py -q — 4 passed.
• pytest tests/ -q -rs — 44 passed, 0 skipped.
• Mutation A (drop the unit_model conversion) — 1 failed, 3 passed. Was 3 passed before.
• Mutation B (molar fixture in the gas-species test) — 1 failed. Was 1 passed before.
• Independent exact re-derivation of the pinned gas-species literals — matches.
• GitHub checks on a2c80e1: Core tests SUCCESS, Assimulo integration tests SUCCESS. mergeStateStatus CLEAN.

Verdict

The fixes do the job. Both blocking items are resolved in the strong sense — the regression tests now detect the regression — and the two nonblocking items are closed. The single remaining gap is uncovered energy_balance numerics, which belongs to #48/#24 rather than to this PR's #21 scope.

No further review round is warranted: the changes are confined to tests, comments, and line endings, with no behavior change outside the commented lines and no public API or CI logic touched. From my side this is merge-ready; note that as PR author I cannot post a formal approval, and GitHub currently reports no reviewDecision gate and no branch protection on master.

[bernalde]

See inline comments

[bernalde]

Notice that above you were using """

[Mazhar331]

Addressed in beb53e4: the adjacent unit_model docstring now consistently uses triple-double quotes.

[Mazhar331]

Still addressed at current head. The unit_model docstring consistently uses triple-double quotes.

[Mazhar331]

Addressed the latest review feedback in commit beb53e4.

Changes made:

• Expanded _drying_rate_mass_basis to a NumPy-style docstring with parameter/return types, descriptions, and units.
• Made the adjacent unit_model docstring use consistent triple-double quotes.
• Annotated dry_rate_volatiles as [kg/m**3/s].
• Kept the proposed state-storage redesign out of this focused fix; it is an existing broader design concern suitable for separate follow-up.

Tests run:

• conda run -n pharmapy python -m pytest tests/test_drying_model.py -v — 4 passed.
• conda run -n pharmapy python -m pytest tests/ -m "not assimulo" — 33 passed, 11 deselected.
• conda run -n pharmapy python -m pytest tests/ -m assimulo — 11 passed, 33 deselected.

No feedback was declined. CI is currently running on this head. GitHub still reports CHANGES_REQUESTED until the reviewer updates or dismisses the formal review.

[Mazhar331]

Review-resolution check — @bernalde's requested changes

Read-only verification of head beb53e4 against @bernalde's CHANGES_REQUESTED review (submitted on a2c80e1). All four inline comments are resolved in content; the threads remain flagged unresolved only because the formal review has not been dismissed/updated.

@bernalde comment-by-comment

• _drying_rate_mass_basis: "Use numpy doc formatting and add argument description and types" (Drying_Model.py:220) — Addressed. The helper now carries a NumPy-style docstring (Drying_Model.py:219-231 at head) with Parameters (dry_rate : ndarray, [mol/m**3/s]) and Returns (ndarray, [kg/m**3/s]) sections, including types and units.
• "Notice that above you were using """" (Drying_Model.py:226) — Addressed. Both _drying_rate_mass_basis and the adjacent unit_model docstring use triple-double quotes at head; the quote style is now consistent.
• dry_rate_volatiles: "Units?" (Drying_Model.py:313) — Addressed. Annotated as # [kg/m**3/s] at the slice site (Drying_Model.py:321 at head).
• State ordering: "There have to be better ways to store the states... We can create an issue for this to be tackled later" (Drying_Model.py:227) — Addressed as scoped. This was an explicitly-deferred nonblocking suggestion; the author agreed and kept it out of this focused units fix. One loose end: no follow-up issue appears to have been filed yet for the state-structure refactor. Recommend opening one so the deferral is tracked, per @bernalde's own suggestion.

Earlier maintainer (self) review — still holding at head

The two blocking items from the 4029b17 maintainer review were fixed in a2c80e1 and remain intact at beb53e4:

• unit-model-conversion-untested — test_unit_model_converts_drying_rate_before_balance_equations is present (tests/test_drying_model.py:75) and pins the mass-basis rate passed to both balances.
• gas-species-test-tautological — the recomputation was replaced with hand-computed literals (test_material_balance_uses_mass_drying_rate_for_gas_species, tests/test_drying_model.py:139).

The prior in-PR verification comment recorded the mutation checks (dropping the conversion line and feeding molar fixtures now both fail, where they previously passed).

CI evidence

On head beb53e4: Core tests SUCCESS, Assimulo integration tests SUCCESS. mergeStateStatus is CLEAN.

Remaining items

• No blocking items outstanding. No @bernalde comment was declined.
• File a tracking issue for the deferred state-structure refactor (nonblocking).
• The energy-balance numerics (sensible_heat, drying_terms) remain covered only at the contract level via a stubbed energy_balance; the numeric coverage is deferred to High: [Drying] Energy terms multiply molar drying rates by mass-basis heat quantities #48/High: [Drying] Condensed-phase energy balance doubles latent heat #24, consistent with the PR scope.

Next action

No further review round is warranted on technical grounds — the changes since the review are confined to docstrings, a units comment, and line endings, with no behavior change outside the commented lines. The one required human action is on @bernalde's side: GitHub still reports reviewDecision: CHANGES_REQUESTED, so the formal review needs to be updated or dismissed to clear the gate.

_{Read-only assessment; no files, checks, or review state were modified.}

[Mazhar331]

Addressed the latest unit/comment feedback in 70b5ba7 and updated the PR body with a Unit Contract section.

Changes made:

• Added NumPy-style docs for get_drying_rate, unit_model, and material_balance, including argument types and units.
• Documented that get_drying_rate remains molar [mol/m**3/s], while unit_model converts once and the balances consume mass-basis rates [kg/m**3/s].
• Added inline unit comments directly on the material-balance definitions, including dimensionless [-] markers for gas holdup terms.
• Preserved Drying_Model.py CRLF line endings.

Validation:

• conda run -n pharmapy python -m pytest tests/test_drying_model.py -q -> 4 passed
• conda run -n pharmapy python -m pytest tests/ -m "not assimulo" -q -> 33 passed, 11 deselected
• conda run -n pharmapy python -m pytest tests/ -q -> 44 passed
• git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --cached --check -> passed
• GitHub CI on 70b5ba7: Core tests and Assimulo integration tests passed

No review comments were declined. Remaining follow-up boundary: #48/#24 still need energy-balance-specific validation before those issues can close. The formal PR review decision still shows CHANGES_REQUESTED until the reviewer updates or dismisses that review.