Fix distillation shortcut reflux calculations

[Mazhar331]

Closes #72.

Summary

• Use the local min_reflux value when a specified positive reflux is below Rmin, avoiding the pre-startup self.min_reflux attribute access.
• Include q_feed in the first Underwood equation target: sum(alpha_i z_i / (alpha_i - phi)) = 1 - q.
• Split the naming between the Underwood-specific calculation (calc_underwood_min_reflux) and the broader shortcut-design estimate (calculate_shortcut_design).
• Keep the old method names (calc_min_reflux, calculate_heuristics) as deprecated compatibility aliases that still work and emit DeprecationWarning with the new method name.
• Expose the default actual-reflux multiplier as reflux_to_minimum_ratio=1.5 [-]. This is a shortcut-design operating default, not part of Underwood's equations.
• Add focused shortcut-design regressions for both issue paths, the configurable reflux multiplier, and the deprecated alias warnings.

Acceptance Criteria

• Positive specified reflux below Rmin falls back to reflux_to_minimum_ratio * min_reflux without raising AttributeError; the default remains 1.5 * min_reflux.
• Minimum reflux changes with feed quality q_feed instead of assuming saturated-liquid feed.
• Regression tests make input and expected-result units explicit.
• Source comments/docs identify dimensionless Underwood quantities, molar-flow terms, and the dimensionless reflux multiplier.
• Public constructor docs use NumPy-style parameter sections with corrected parameter names and units for the changed distillation API.
• Old public method names remain callable but warn that they are deprecated.

Public API / Compatibility

• Canonical names:
  • calc_underwood_min_reflux(...)
  • calculate_shortcut_design(...)
  • reflux_to_minimum_ratio=1.5 on DistillationColumn, DynamicDistillation, and _BaseDistillation
• Deprecated aliases:
  • calc_min_reflux(...) -> calc_underwood_min_reflux(...)
  • calculate_heuristics(...) -> calculate_shortcut_design(...)
  • column_startup(time_heuristics=...) -> column_startup(time_shortcut_design=...)
• The deprecated aliases are intentionally retained so existing callers do not break immediately.

Units And Dimensional Analysis

• alpha, phi, feed/product mole fractions, q_feed, 1 - q_feed, reflux ratios, and reflux_to_minimum_ratio are dimensionless [-].
• The first Underwood residual subtracts dimensionless quantities only.
• dist_flowrate, V_min, and L_min are molar flow rates [mol/s].
• min_reflux = L_min / D is dimensionless [-].
• perc_LK and perc_HK are documented as percentages [%] in both steady-state and dynamic distillation constructor docs.

Review Follow-Up Scope

• tests/assimulo_helpers.py now owns a reusable import_module_with_assimulo_stub(...) helper.
• tests/test_cryst_kinetics.py uses that shared helper instead of maintaining its own local Assimulo stub copy.
• That crystal-kinetics test change was requested by review to avoid duplicate optional-dependency stub logic; it does not change crystallizer or kinetics behavior.
• tests/test_distillation_shortcut_design.py uses the same helper for the distillation Assimulo imports.

Related Issues

• Critical: [Distillation] DistillationColumn.solve_unit calls retrieve_results without required arguments #49 and Medium: [Distillation] VLE calls drop gamma_model and use ideal activity coefficients #59 are distinct distillation defects noted in the issue body; this PR does not change their paths.

Branch Hygiene

• Base branch: master
• Source branch: fix/issue-72-distillation-underwood
• Branch point: origin/master at 0f89b71999daf9b765ff80255a61ed73e9e9f0e5
• Head commit: 35feb029f159b444b6e3988848d8584cee88b96b
• Stacked status: not stacked; no prerequisite PRs
• Open PR overlap: checked current open PR changed files; none touch PharmaPy/Distillation.py, tests/assimulo_helpers.py, tests/test_cryst_kinetics.py, or tests/test_distillation_shortcut_design.py.

Local Checks

• Red check before fix: /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation_shortcut_heuristics.py -q failed with the issue-expected AttributeError and Rmin = 0.527777... instead of 0.7.
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation_shortcut_design.py tests/test_distillation.py -q -> 4 passed
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -m "not assimulo" -q -> 43 passed, 10 deselected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest --collect-only -q -> 53 tests collected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -q -> 53 passed
• git diff --check
• git diff --cached --check
• GitHub Actions on 35feb02: Core tests SUCCESS, Assimulo integration tests SUCCESS

Review Follow-Up Metadata

• Pushed 35feb02 to existing branch fix/issue-72-distillation-underwood.
• Added NumPy-style docs to Distillation.py constructors, helpers, the nested Underwood residual, steady/dynamic solve paths, result retrieval, and plotting.
• Added docs and bracketed units in the distillation and crystal-kinetics tests, including local test-double methods and fixture values.
• Merge state was already clean; no conflicts were present.
• Verification: docstring audit clean; unit audit clean; targeted tests 11 passed; full suite 75 passed, 2 warnings; diff check passed.

[Mazhar331]

Maintainer review of the shortcut-design reflux fixes for #72. I authored this PR, so this is a COMMENT review — see the merge-readiness note at the bottom.

Both defects described in the issue are fixed at the right place, and I verified each one independently rather than relying on the green suite.

Verification

Both regression tests are genuinely red without their fix. Because PharmaPy is editable-installed here (a __editable__ import finder wins over PYTHONPATH), a worktree-plus-PYTHONPATH mutation check would have silently exercised the original source, so I mutated the real file in place and restored it afterwards (tree confirmed clean, HEAD still cde248b):

• Reverting reflux = 1.5 * min_reflux to 1.5 * self.min_reflux fails test_positive_reflux_below_minimum_uses_current_minimum with exactly the symptom the issue reports — AttributeError: 'DistillationColumn' object has no attribute 'min_reflux' — not a downstream error absorbed by a broad raises.
• Reverting underwood_target to 0 fails test_calc_min_reflux_includes_feed_quality_in_underwood_target with Rmin = 0.5277777 vs the expected 0.7.

The expected value is independent of the implementation. I derived 0.7 by hand rather than trusting the code: with alpha = [4, 1], z = [0.4, 0.6], q = 0.8, the first Underwood equation 1.6/(4-phi) + 0.6/(1-phi) = 0.2 is solved exactly by phi = 2, giving V_min = 36/2 - 1 = 17 mol/s, L_min = 7 mol/s, Rmin = 0.7 [-]. The pre-fix root phi = 1.8181 likewise reproduces 0.5277 analytically. The assertion pins a value, not a shape or count.

The 1 - q convention matches this repo's q_feed. I checked every consumer rather than assuming the textbook sign: both constructor docstrings define q_feed as "fraction of liquid in the feed stream in molar basis", and calc_plates:412 (Lm = Ln + F + F*(q_feed-1)) and material_balances:725 (Lm = Ln + F*q_feed) both use it as the liquid fraction. So sum(alpha*z/(alpha-phi)) = 1 - q_feed is the correct target, and q_feed = 1 reduces to the previous residual exactly — no behavior change for existing saturated-liquid callers, which tests/test_distillation.py (q_feed=1.0) confirms still passes.

The new residual is well-conditioned across feed qualities. Since the PR materially changes the objective, I swept q from 1.5 to -0.2 on the test fixture: minimize reports success at every point with a residual around 1e-16, and Rmin rises monotonically (0.263 at q=1.5 → 0.700 at q=0.8 → 1.944 at q=0.0 → 2.344 at q=-0.2), which is the physically expected direction as the feed vaporizes. I had considered flagging the unchecked minimize success flag, but it does not appear to be a practical risk on this residual and is pre-existing, so I am not raising it.

No latent second reader of the uninitialized attribute. calc_plates:523 also reads self.min_reflux, but solve_unit assigns it at line 529 before calling calc_plates, so that path is safe.

Findings

One nonblocking finding, posted inline on tests/test_distillation_shortcut_heuristics.py: the assimulo-stub helper is now duplicated from tests/test_cryst_kinetics.py and could be consolidated into tests/conftest.py.

No blocking findings and no open questions.

Scope And Issue Coverage

Closes #72 is the correct link: the issue enumerates exactly two defects and this PR fixes both. #49 (retrieve_results arity) and #59 (dropped gamma_model) are explicitly called out in the issue as related-but-distinct, and this PR correctly leaves them alone. The phi.x[0] and sum → np.sum cleanups are behavior-preserving on the lines already being touched.

The tests target calculate_heuristics and calc_min_reflux directly rather than a full solve_unit column. That is the right scope here — the AttributeError is raised inside calculate_heuristics, which solve_unit calls first, so a heavier end-to-end fixture would add brittleness without covering more of the defect.

Tests Run

All in the repo's pharmapy conda environment, against PR head cde248b:

• pytest tests/test_distillation_shortcut_heuristics.py -q → 2 passed
• pytest tests/test_distillation.py -q → 1 passed
• pytest tests/ -q (full suite, assimulo available) → 52 passed, 2 pre-existing unrelated warnings
• Mutation checks (both reverts) → red, as detailed above

No skipped or deselected tests in the full local run, since assimulo is installed here. CI covers the complementary case: Core tests runs pytest tests/ -m "not assimulo" in an environment without assimulo, which is what actually exercises the new file's stub path — and it passed. No test residue beyond already-gitignored __pycache__.

Merge Readiness

GitHub checks on head cde248b: Core tests SUCCESS, Assimulo integration tests SUCCESS. The head sits directly on origin/master (0 commits behind), so these PR-head results are also the merge-result state; mergeStateStatus is CLEAN and mergeable is MERGEABLE.

There are no blocking findings. This is a COMMENT rather than an APPROVE only because GitHub rejects an approving review from the PR author — this account cannot provide a formal approval. master currently reports no branch protection and reviewDecision is empty, so GitHub is not reporting an approving-review gate on this PR.

[Mazhar331]

Addressed the open review feedback on PR #121.

Commits pushed:

• ec17215 Consolidate assimulo test import stubs

Changes made:

• Moved the configurable Assimulo module stub into tests/conftest.py.
• Updated tests/test_cryst_kinetics.py and tests/test_distillation_shortcut_heuristics.py to share import_module_with_assimulo_stub(...) with per-module solver/problem attributes.
• Verified the explicit missing-Assimulo import path with a throwaway import check.

Tests and checks:

• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation_shortcut_heuristics.py tests/test_cryst_kinetics.py -q -> 7 passed
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest --collect-only -q -> 52 tests collected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -m not\ assimulo -q -> 42 passed, 10 deselected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -q -> 52 passed
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python /tmp/check_assimulo_stub_import.py -> assimulo stub import path ok
• git diff --check
• GitHub Actions on ec17215: Core tests pass, Assimulo integration tests pass

Comments intentionally not addressed:

• None.

Remaining risks / gates:

• No blocking review findings remain from this batch.
• reviewDecision is still empty because the review was a COMMENT review; the PR author account cannot formally approve its own PR.

[Mazhar331]

Review-resolution verification at ec17215

Read-only verification pass over the review feedback on this PR. One review comment was open; it is Addressed, and I confirmed the fix at the current head rather than relying on the address summary's own green test log.

Comment-by-comment status

Nonblocking — assimulo stub helper duplicated from tests/test_cryst_kinetics.py (#discussion_r3597393084) — Addressed in ec17215.

The consolidation is what the comment asked for and is done correctly:

• Both duplicate copies are gone. _stub_assimulo_modules now lives once in tests/conftest.py, parameterized by the solvers / problem attributes to stub, with import_module_with_assimulo_stub(...) wrapping the ModuleNotFoundError retry.
• Both call sites use it with their own configuration — CVode/Explicit_Problem for PharmaPy.Crystallizers, IDA/Implicit_Problem for PharmaPy.Distillation. Net -61/+51 lines, and the helper sits next to the existing _has_assimulo / pytest_collection_modifyitems logic that already owned assimulo knowledge.

The regression guarantee survives the refactor

ec17215 edits the test files, so the fact that the tests are green at head does not by itself prove they still catch the original bugs — a green log only ever runs against the fixed source. I re-ran both mutation checks against the head tests.

Because PharmaPy is editable-installed (its import finder beats PYTHONPATH), I injected a mutated copy of Distillation.py into sys.modules from a scratch directory, asserting the loaded __file__ was the scratch path so the real source could not silently win. Nothing in the checkout was modified.

• Reverting reflux = 1.5 * min_reflux → 1.5 * self.min_reflux still fails test_positive_reflux_below_minimum_uses_current_minimum with the issue's exact symptom: AttributeError: 'DistillationColumn' object has no attribute 'min_reflux'.
• Reverting underwood_target → 0 still fails test_calc_min_reflux_includes_feed_quality_in_underwood_target with Obtained: 0.5277777 vs Expected: 0.7.

PharmaPy/Distillation.py is byte-identical between cde248b and ec17215 (git diff --stat empty), so the production fix verified in the previous review is untouched and the change is confined to test import plumbing.

CI evidence

The decisive check is the assimulo-free lane, since the stub path is dead code wherever assimulo is installed. In run 29518312984 on ec17215, Core tests (pytest tests/ -m "not assimulo", no assimulo present) collected and passed both call sites of the shared helper:

tests/test_cryst_kinetics.py .....                                       [ 64%]
tests/test_distillation_shortcut_heuristics.py ..                        [ 69%]
============ 42 passed, 3 skipped, 6 deselected, 7 warnings in 1.29s ===========

Assimulo integration tests also passed. Locally: pytest tests/ -q → 52 passed.

Remaining issues

None blocking. One minor observation, not worth a change on its own: from conftest import import_module_with_assimulo_stub imports conftest directly, which pytest's docs generally discourage. It is unambiguous here — a single tests/conftest.py, no __init__.py, and the default prepend import mode — and it is proven working in both CI lanes. It would only become fragile if the repo later adopted importmode=importlib or added a second same-named conftest in a non-package directory; a tests/helpers.py module would sidestep that. Fine to leave as is.

Verdict

No further review round is warranted. The fix is confined to the test-import plumbing the comment explicitly pointed at, introduces no production behavior or public API change, and the touched sibling test (test_cryst_kinetics.py) is verified green in both environments.

[bernalde]

Shouldn’t the function name tell you somehow the method used? At the end of the day Underwood’s equations are a heuristic

[Mazhar331]

Addressed in b6427ba: the Underwood-specific calculation is now calc_underwood_min_reflux(...), while the broader estimate is calculate_shortcut_design(...). The previous names remain as compatibility aliases.

[Mazhar331]

Addressed in 73e6977: split the Underwood minimum-reflux method from the broader shortcut-design method and retained deprecated aliases.

[Mazhar331]

Follow-up in c643472: 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/Distillation.py tests/assimulo_helpers.py tests/test_cryst_kinetics.py tests/test_distillation.py tests/test_distillation_shortcut_design.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation.py tests/test_distillation_shortcut_design.py -q -> 4 passed; git diff --cached --check -> passed.

[Mazhar331]

Addressed in e1a7ce7: kept the method separation documented after the merge-forward update.

[bernalde]

The 1.5 is a magic number. Does it explicitly appear in Underwood’s equation or is it a parameter we should expose to changing and set the default to 1.5?

[Mazhar331]

Addressed in b6427ba: 1.5 is not part of Underwood's equations. It is now reflux_to_minimum_ratio=1.5 [-], the default shortcut-design multiplier for choosing actual reflux above Rmin, and tests cover changing it.

[Mazhar331]

Addressed in 73e6977: exposed the default as reflux_to_minimum_ratio=1.5 [-] instead of keeping it as an embedded Underwood constant.

[Mazhar331]

Follow-up in c643472: 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/Distillation.py tests/assimulo_helpers.py tests/test_cryst_kinetics.py tests/test_distillation.py tests/test_distillation_shortcut_design.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation.py tests/test_distillation_shortcut_design.py -q -> 4 passed; git diff --cached --check -> passed.

[Mazhar331]

Addressed in e1a7ce7: kept reflux_to_minimum_ratio=1.5 [-] exposed and covered by literal expectations.

[Mazhar331]

Addressed the latest review feedback on PR #121.

Commits pushed:

• b6427ba Clarify distillation shortcut design naming

Changes made:

• Added calc_underwood_min_reflux(...) for the Underwood minimum-reflux calculation and calculate_shortcut_design(...) for the broader Fenske/Underwood/Gilliland-Kirkbride shortcut estimate.
• Kept calc_min_reflux(...) and calculate_heuristics(...) as backward-compatible aliases.
• Exposed the default operating reflux multiplier as reflux_to_minimum_ratio=1.5 [-] and documented that it is not part of Underwood's equations.
• Renamed the shortcut test file and added coverage for a configured reflux multiplier plus compatibility aliases.
• Updated the PR title to Fix distillation shortcut reflux calculations.

Tests and checks:

• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation_shortcut_design.py tests/test_distillation.py -q -> 4 passed
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest --collect-only -q -> 53 tests collected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -m "not assimulo" -q -> 43 passed, 10 deselected
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -q -> 53 passed
• git diff --check
• git diff --cached --check
• GitHub Actions on b6427ba: Core tests SUCCESS, Assimulo integration tests SUCCESS

Comments intentionally not addressed:

• None.

Remaining risks / gates:

• The latest implementation comments are addressed, but reviewDecision remains CHANGES_REQUESTED until the reviewer updates or dismisses that review.
• I did not mark review threads resolved.

[Mazhar331]

Review-resolution verification at d49d167

Read-only verification pass covering @bernalde's CHANGES_REQUESTED review (4722138259) and the earlier nonblocking comment. All three review threads are content-Addressed at the current head, and I verified each at d49d167 rather than relying on the address summary's own log — which only referenced b6427ba, one commit behind the current head.

I materialized the exact head tree with git archive d49d167 (the local checkout is on another branch, so this avoids a wrong-source run). Because PharmaPy is editable-installed against a checkout on master, import PharmaPy.Distillation resolves to master by default; I confirmed that trap directly (a plain run of the head tests failed with AttributeError: ... has no attribute 'calc_underwood_min_reflux' against master source) and ran every check with the head module injected under the real dotted name so the intended source is exercised.

Comment-by-comment status

@bernalde — function name should reflect the method (r3602689379) — Addressed in b6427ba. The Underwood-specific routine is now calc_underwood_min_reflux(...) and the broader Fenske/Underwood/Gilliland-Kirkbride estimate is calculate_shortcut_design(...). calc_min_reflux / calculate_heuristics remain as thin backward-compatible aliases that delegate to the new names, and all internal callers (solve_unit, column_startup) use the canonical names, so the aliases exist purely for external compatibility rather than masking the method in use.

@bernalde — 1.5 is a magic number; expose it as a parameter (r3602695305) — Addressed in b6427ba. 1.5 is now reflux_to_minimum_ratio (constructor kwarg, default DEFAULT_REFLUX_TO_MINIMUM_RATIO = 1.5, documented as an operating-reflux default that is not part of Underwood's equations), applied in both the reflux is None/0 and reflux < Rmin branches. test_positive_reflux_below_minimum_uses_configured_shortcut_ratio sets reflux_to_minimum_ratio=1.8 and asserts reflux == 1.8 * Rmin, so the parameter is genuinely exercised, not just accepted.

Nonblocking — assimulo stub helper duplicated (r3597393084) — Addressed in ec17215 (verified in the previous pass; thread now isResolved: true). Unchanged since.

The original #72 regression guarantees survive the rename

b6427ba renamed the production functions and d49d167 edited docstrings, so a green head log does not by itself prove the tests still catch the original bugs. I re-ran both mutation checks against the injected head module:

• Reintroducing defect 1 (revert the operating-reflux line to self.reflux_to_minimum_ratio * self.min_reflux) fails test_positive_reflux_below_minimum_uses_configured_shortcut_ratio with the issue's exact AttributeError: 'DistillationColumn' object has no attribute 'min_reflux'.
• Reintroducing defect 2 (underwood_target = 0) fails test_underwood_min_reflux_includes_feed_quality_in_target with Obtained: 0.5277777 vs Expected: 0.7.

The q_feed target (1 - self.q_feed) and the local-variable reflux fix are both intact at head; the change since the previous verified head is confined to the rename, the new parameter, and docstrings.

Tests / CI evidence

• Injected unmutated head module → the three shortcut-design tests pass; both mutations red as above.
• d49d167 is docstring-only for Distillation.py (stale x_LK/x_HK constructor param docs corrected to perc_LK/perc_HK); no behavior change.
• test_distillation.py was updated to monkeypatch the renamed calculate_shortcut_design, which solve_unit actually calls, so that test is not vacuous.
• GitHub Actions CI on d49d167: Core tests SUCCESS, Assimulo integration tests SUCCESS.

Remaining issues

None on the content. The open item is procedural: this is new public API (two new method names plus aliases, and a new reflux_to_minimum_ratio constructor kwarg on DistillationColumn, DynamicDistillation, and the base class), which is broader than a rename in place.

Another review round?

reviewDecision is CHANGES_REQUESTED and, per GitHub, that is held specifically by @bernalde's review — addressing the comments and the author's replies do not clear it, and no other account's approval can override it. master has no branch protection (the protection endpoint 404s), so this is not a branch-protection approval gate; it is @bernalde's standing verdict. The right next step is therefore a human re-review by @bernalde, not another automated round: the new public API is exactly the kind of change worth a second look, and only @bernalde re-reviewing or dismissing 4722138259 will move reviewDecision off CHANGES_REQUESTED.

@bernalde — both of your comments are addressed at d49d167 (renamed methods with compatibility aliases; 1.5 exposed as reflux_to_minimum_ratio), and the two threads are still marked unresolved. When you have a moment, could you re-review and either resubmit or dismiss 4722138259? That is the only action that lifts the CHANGES_REQUESTED gate.

[Mazhar331]

Addressed the PR #121 review feedback in commit 73e6977.

Changes made:

• Replaced the reviewed test_steady_state_solve_uses_calc_plates_results monkeypatches with a small subclass test double that exercises the real solve_unit orchestration branch.
• Moved optional Assimulo import stubbing out of conftest.py into documented tests/assimulo_helpers.py; the helper only stubs import symbols when Assimulo is absent.
• Updated the crystallization and distillation tests to use the shared helper instead of direct conftest imports.
• Normalized touched distillation test units to [...].
• Kept the existing Underwood/reflux monkeypatches only as focused collaborator-boundary fixtures for the shortcut-design branch tests.

Tests run:

• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation.py tests/test_distillation_shortcut_design.py -q -> 4 passed.
• /home/mlaljee/miniforge3/bin/python -m pytest tests/test_distillation_shortcut_design.py tests/test_cryst_kinetics.py -q -> 8 passed.
• /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/ -q -> 53 passed.
• git diff --cached --check -> passed.

Post-push CI on 73e6977: Core tests SUCCESS; Assimulo integration tests SUCCESS. mergeStateStatus is CLEAN. No review comments were intentionally declined. The formal review state remains CHANGES_REQUESTED until @bernalde re-reviews or dismisses the prior review.

[bernalde]

monkeypatches all over the place

[Mazhar331]

Addressed in 73e6977: the reviewed solve_unit test no longer monkeypatches instance methods. It uses a small subclass to exercise the real orchestration branch and stops before unrelated result retrieval.

[Mazhar331]

Follow-up in c643472: 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/Distillation.py tests/assimulo_helpers.py tests/test_cryst_kinetics.py tests/test_distillation.py tests/test_distillation_shortcut_design.py -> passed; /home/mlaljee/miniforge3/envs/pharmapy/bin/python -m pytest tests/test_distillation.py tests/test_distillation_shortcut_design.py -q -> 4 passed; git diff --cached --check -> passed.

[Mazhar331]

Addressed at e1a7ce7: the reviewed tests now use subclass doubles where practical; the remaining alpha-equilibrium stub only isolates the Underwood numeric formula.

[Mazhar331]

Pushed 35feb02.

Changes:

• Added NumPy-style docs to Distillation.py, including constructors and the nested Underwood residual.
• Added docs and bracketed units across the distillation and crystal-kinetics tests.
• No merge conflicts were present.

Checks: docstring audit clean; unit audit clean; targeted tests 11 passed; full suite 75 passed, 2 warnings; diff check passed.

[Mazhar331]

Read-only verification pass at c643472

Verification-only pass over the review feedback on this PR. No formal approval — this records per-item resolution and an explicit units/documentation/comments audit. Head re-resolved fresh as c643472b760bdfee3a8bc593e41912b25bcac655. Base master (c7a38a4) has moved since the branch point 0f89b71, but only in PharmaPy/Drying_Model.py and drying tests, so there is no overlap with this PR.

Three of four comments are addressed; one is partially addressed. No comment was declined. One new regression was introduced by the most recent commit and should be fixed before re-review.

Comment-by-comment status

Nonblocking — assimulo stub helper duplicated (r3597393084) — Addressed. The helper lives once at tests/assimulo_helpers.py:8,46 with Numpy-format docstrings, consumed by tests/test_cryst_kinetics.py:6,22-28 and tests/test_distillation_shortcut_design.py:6,13-18; the duplicate copy is deleted. Note the thread reply and the PR body both still say it lives in tests/conftest.py — it moved to tests/assimulo_helpers.py in 73e6977, so the PR body needs a correction.

@bernalde — function name should reflect the method (r3602689379) — Addressed. calc_underwood_min_reflux at PharmaPy/Distillation.py:261, calculate_shortcut_design at :342, deprecated aliases at :323-340 and :443-458. All internal callers use the canonical names (:377, :655, :807). A repo-wide grep over .py/.ipynb/.md finds no in-repo consumer of the old names outside tests, so the aliases break nothing.
One unannounced signature change: column_startup's parameter was renamed time_heuristics → time_shortcut_design (:806) with no alias. The only in-repo call site passes positionally (:920), so it is safe, but it is missing from the PR's compatibility map.

@bernalde — 1.5 is a magic number (r3602695305) — Addressed, with a coverage caveat. DEFAULT_REFLUX_TO_MINIMUM_RATIO = 1.5 # [-] at :24-26, threaded through all three constructors (:33, :465, :703), documented at :504-508 and :738-742, applied at :389 and :398.
Caveat: no test pins the 1.5 default — see the mutation table below. Only the non-default 1.8 is exercised. Separately, the same magic-number concern still applies elsewhere on this call path: the Kirkbride exponent 0.206 (:254) and the Gilliland-Molokanov constants 54.4/117.2/11 (:232) are bare literals in functions with no docstring. Those are genuine literature constants, unlike 1.5, but nothing in the code says so.

@bernalde — "monkeypatches all over the place" (r3617925156) — Partially addressed. The flagged file is clean: tests/test_distillation.py has zero monkeypatch and uses a subclass double (:32-52). But the new tests/test_distillation_shortcut_design.py added by the same PR uses six monkeypatch.setattr calls (:59, :60-61, :62, :97-98, :99-100, :139). In test_positive_reflux_below_minimum_uses_configured_shortcut_ratio all three collaborators are stubbed, so no shortcut-design numerics actually run; in test_backward_compatible_shortcut_aliases the delegate itself is stubbed, so that test pins delegation only. The mechanism in the flagged file changed; the overall degree of test-doubling did not. Whether that answers the concern is @bernalde's call.

Test evidence: are the fixes actually guarded?

I checked out the exact head tree into a scratch directory (nothing in the working checkout was modified) and put each original bug back, one at a time, to see whether the tests notice. A test failing here is the good outcome — it means the test is really guarding the fix rather than passing for unrelated reasons.

• Put back bug 1 (change :398 back to self.reflux_to_minimum_ratio * self.min_reflux): the test failed, with exactly the error issue High: [Distillation] Shortcut design crashes below Rmin and ignores q_feed in Underwood calculation #72 reports — AttributeError: 'DistillationColumn' object has no attribute 'min_reflux' — raised at the buggy line itself, not swallowed somewhere downstream. Good.
• Put back bug 2 (change :299 back to underwood_target = 0): the test failed, reporting 0.5277777 where 0.7 is expected. Good.
• Flip the feed-quality sign (q − 1 instead of 1 − q at :299): the test failed (0.4006), which confirms the q-line convention is pinned by an actual number, not just by the shape of the result. Good.
• Change the documented 1.5 default to 2.5 (:26): everything still passed. This one is a gap — nothing in the suite would notice if the documented default silently changed. The reflux is None or == 0 branch (:386) and the reflux < 0 branch (:390) are uncovered too.

The expected value 0.7 is independent of the code under test. With alpha = [4, 1], z = [0.4, 0.6], q = 0.8, the first Underwood equation 1.6/(4−φ) + 0.6/(1−φ) = 0.2 has the exact root φ = 2, and the residual is strictly increasing over the bound interval [1, 4], so the root is unique and well-posed. Then V_min = 17 mol/s, L_min = 7 mol/s, R_min = 0.7 [-]. I re-derived this by hand and it matches. By contrast, tests/test_distillation_shortcut_design.py:67-68 asserts column.reflux_to_minimum_ratio * min_reflux rather than the literal 3.6, i.e. it restates the production expression; a literal would be stricter.

The 1 − q convention is right for this repo, not just for the textbook: both constructors define q_feed as the molar liquid fraction (:475, :711), and both consumers use it that way — calc_plates:540 (Lm = Ln + F + F*(q_feed − 1)) and material_balances:866 (Lm = Ln + F*q_feed). At q_feed = 1 the residual reduces exactly to the old target of 0, so existing saturated-liquid callers are unaffected, which tests/test_distillation.py confirms.

CI evidence

Run 29867674225 on c643472, both jobs SUCCESS:

• Core tests (no Assimulo installed): 43 passed, 4 skipped, 6 deselected, including tests/test_distillation_shortcut_design.py ... — so the new tests and the Assimulo import-stub fallback are genuinely exercised in the environment that lacks the optional dependency.
• Assimulo integration tests: 14 passed, 43 deselected, including test_steady_state_solve_uses_calc_plates_results PASSED.

All seven branch runs from cde248b through c643472 are green. I triggered no runs.

Units / documentation / comments audit

1. Regression introduced by c643472 — please fix before re-review. PharmaPy/Distillation.py:93 now sets 'units': '[K]', and :96/:105 add 'units': '[-]'. Every other module uses bare values: 'K' (Containers.py:77,211, Streams.py:67, Reactors.py:332, Crystallizers.py:389, DynamicExtraction.py:86), 'kg/s', 'mol', '' for dimensionless (Evaporators.py:553,555, Containers.py:885). Distillation.py is now the only file in the package with bracketed metadata.

This is not cosmetic — PharmaPy/Plotting.py:184-187 and :267-270 concatenate that value straight into the axis label as ylabel + ' (' + latexify_name(units, units=True) + ')'. Rendering check:

'K'   -> temp ($\mathregular{K}$)     # before
'[K]' -> temp ($\mathregular{[K]}$)   # after, renders as: temp ([K])

and x_liq/y_vap previously had no 'units' key at all, so len(units) > 0 was false and no suffix was drawn; they now gain a ([-]) suffix. DynamicDistillation.plot_profiles (:1036) → plot_distrib → name_yaxes is the live path. The bracket convention is right for comments and docstrings — where this PR applies it well — but not for states_di values that get string-concatenated into labels. Suggest reverting :93, :96, :105 to 'K'/'', or making the bracket convention a separate repo-wide change that updates Plotting.py too.

2. Adjacent call path is undocumented. The changed hunks are annotated well, but calculate_shortcut_design calls three functions that received nothing:

• calc_num_min (:215-227) — no docstring; alpha_top, alpha_bottom, alpha_fenske, num_min all [-], unannotated.
• molokanov_equation (:229-237) — no docstring; x_val, y_val, num_stages all [-].
• kirkbride_correlation (:239-259) — no docstring; num_ratio, num_rect, num_strip all [-].

Also undocumented in the touched file: VLE (:520), calc_plates (:530-652, with Ln/Vn/Lm/Vm [mol/s] unannotated), DistillationColumn.solve_unit (:654), retrieve_results (:673), column_startup (:806), material_balances (:849-898, with M_const [mol], dx_dt [1/s], residuals_temp [-]), energy_balances (:900), and all three class-level docstrings.

Two annotations are ambiguous rather than wrong: :383 (mat_bce) and :439 (out) both carry a lumped # [-], [mol/s] that does not say which key has which unit. Per-key units or a Returns block naming the keys would fix it.

The calc_underwood_min_reflux docstring (:263-291) is good but omits two things: the residual form itself (Σ αᵢzᵢ/(αᵢ − φ) = 1 − q) and the fact that alpha is evaluated at the feed composition and its bubble point (:298), normalized to the heavy key.

3. Quantity mismatch in adjacent untouched code (pre-existing). :690-695 builds the outlet streams with mole_conc=dist_result['x_dist'] and mole_conc=dist_result['x_bot'], but those are mole fractions [-] going into a mole concentration argument [mol/m³]. DynamicDistillation.retrieve_results (:1024-1032) does it correctly with mole_frac=. This sits on the solve_unit → calc_plates → retrieve_results path this PR changes. Outside this diff — suggest a follow-up issue rather than expanding scope here.

4. Untested branches and one latent edge case. Beyond the 1.5 default: reflux is None or == 0 (:386), reflux < 0 (:390), the Gilliland path (:404), and the Kirkbride path (:419) are uncovered. On the sibling branch of the fixed one, reflux = -self.reflux * min_reflux (:391) with abs(self.reflux) < 1 yields reflux < min_reflux, which makes x_val negative at :230 and np.sqrt(x_val) NaN at :233, propagating to np.ceil(NaN) and an eventual int(NaN) failure in column_startup:811. Unguarded and untested.

5. Minor. :304 guards the first Underwood equation with + np.finfo(float).eps but :316 does not; now that the residual has a nonzero target, that offset biases it asymmetrically. scipy.optimize.minimize's success flag remains unchecked (:311-313) — already acknowledged and swept over q ∈ [−0.2, 1.5] without failure, so a note only.

Remaining issues

1. states_di bracketed units regression (:93, :96, :105) — introduced by c643472, user-visible through Plotting.py, inconsistent with every other module.
2. The documented 1.5 default is unpinned by any test, and the reflux is None/reflux < 0 branches are uncovered.
3. Missing docstrings/units on calc_num_min, molokanov_equation, kirkbride_correlation, calc_plates, material_balances and the other helpers listed above.
4. mole_conc= vs mole_frac= at :690-695 — pre-existing, follow-up issue.
5. PR body is stale: helper location (tests/assimulo_helpers.py, not tests/conftest.py) and the column_startup parameter rename missing from the compatibility map.

Another review round?

Not another automated one — this is the third verification pass on this PR (ec17215, d49d167, c643472), and the remaining items are judgement calls plus one concrete regression rather than something a further loop resolves.

@bernalde — reviewDecision is CHANGES_REQUESTED, held specifically by your review 4739390563. master has no branch protection, so this is not an approval-count gate: only you re-reviewing or dismissing that review lifts it, and neither the author's replies nor another maintainer's approval can substitute. A re-review is independently warranted here — since your last review the PR has materially restructured test logic (73e6977) and changed user-visible plot metadata (c643472), which is beyond what the original comments anticipated. Suggest the author fixes item 1 above first.

[bernalde]

Please resolve the merge conflicts and aim to reduce the usage of monkeypatch