Fix semibatch reactor energy flow term

[parkyr]

Summary

• Correct the semibatch reactor convective energy term so sensible enthalpy at reactor temperature is weighted by inlet composition, matching the semibatch material accumulation balance with no outlet stream.
• Preserve CSTR behavior by keeping the holdup-composition outflow enthalpy for reactors without the semibatch vol state.
• Add algebra-level regression tests for the semibatch zero-driving-force case and the CSTR outflow guard.

Closes #87.

Related Issues / Priority

• The Assimulo import workaround in the new algebra-level test is temporary and directly related to Decouple the ODE/DAE solver behind an interface (assimulo optional, scipy fallback) #10, which tracks lazy solver imports plus a solver interface / SciPy fallback.
• Decouple the ODE/DAE solver behind an interface (assimulo optional, scipy fallback) #10 should be prioritized before broad solver-adjacent test expansion or larger unit-operation refactors, because it removes the need for import-time stubs in core tests.
• Decouple the ODE/DAE solver behind an interface (assimulo optional, scipy fallback) #10 does not need to block this PR: this PR is a focused correctness fix for High: [Reactors] Semibatch energy balance subtracts fictitious outflow enthalpy #87, keeps CSTR behavior pinned, and passes both core and Assimulo CI.
• Related roadmap context: [Epic] Restore CI, raise the SE baseline, and extend modeling (bioreactors, crystallizers, scheduling) #3 covers the broader CI / engineering baseline, Expand test coverage across modules (unit + integration) #7 covers test expansion, and Modernize packaging and project metadata (pyproject.toml, drop egg-info) #8 covers packaging metadata cleanup.

Tests

• conda run -n pharmapy python -m pytest tests/test_reactors_energy.py -q
• conda run -n pharmapy python -m pytest --collect-only
• conda run -n pharmapy python -m pytest tests/ -m "not assimulo"
• timeout 180s conda run -n pharmapy python -m pytest tests/Flowsheet/flowsheet_tests.py::TestFlowsheets::test_SB_BC_FILT -q
• timeout 180s conda run -n pharmapy python -m pytest tests/ -m assimulo -q
• git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --check --cached

Notes: pytest emitted existing invalid-escape deprecation warnings in several modules and one existing runtime warning in the PFR flowsheet test.

Branch Hygiene

• Base branch: master
• Source branch: fix/issue-87-semibatch-energy
• Branch point: origin/master at 1c3a046fceb5c4da8b5fdb6e8b254f657d6e3cc7
• Upstream status: origin/master includes upstream/master at 400e1e13ad4ecd3ba202d5fae8c70ec73bdf9c3e
• Stacked status: not stacked; no prerequisite PRs

[parkyr]

Maintainer review — Fix semibatch reactor energy flow term (#102, closes #87)

Reviewed head f8dad35 against base 1c3a046. The fix is correct, minimal, and well-scoped. Posting as COMMENT because I authored this PR (GitHub rejects an author APPROVE).

Correctness — verified

The change branches the convective second-enthalpy term on 'vol' in self.states_uo, weighting h_temp by inlet composition for the semibatch (no outlet stream) and keeping holdup composition for the CSTR outflow. I independently confirmed:

• SemibatchReactor.nomenclature is the only path that appends 'vol' to states_uo, and it runs during __init__ (via CSTR.__init__ → self.nomenclature()), so the guard is True for a semibatch and False for CSTR/Batch/PFR both at construction and in the real solve. The fix also reuses the exact idiom already present at Reactors.py:1084.
• Regression tests are not vacuous: run against true base source (1c3a046), test_semibatch_flow_term_... fails with -1.037e7 ≠ 0 and passes on head; test_cstr_flow_term_keeps_holdup_composition_for_outflow asserts -1.037e7 and passes on both base and head, so it locks in "no CSTR regression" (issue #87 acceptance criterion #3).

Tests run locally (env pharmapy, assimulo present)

• tests/test_reactors_energy.py — 2 passed (head); semibatch case fails on base as expected.
• pytest tests/ -m "not assimulo" — 29 passed, 6 deselected.
• test_SB_BC_FILT (assimulo integration) — passed.
• CI on this head: Core tests SUCCESS, Assimulo integration tests SUCCESS.

Warnings observed are pre-existing invalid-escape DeprecationWarnings in unrelated modules; not introduced here.

Nonblocking

Nonblocking: The integration test test_SB_BC_FILT still only asserts the solve completes (no assertion on the corrected temperature trajectory), so the full-solve path would not catch a future regression of this term — the new algebra tests are what guard it. Consider adding a lightweight assertion there (e.g. an energy-balance/temperature check or a golden trace) so the integration path also protects the fix. The unit-level coverage already makes this optional.

Nonblocking: tests/test_reactors_energy.py stubs assimulo and rewrites sys.path/sys.modules (_prefer_source_package, _stub_assimulo_modules) so the algebra test can import PharmaPy.Reactors in the no-assimulo Core job. This is justified (the top-level from assimulo... import would otherwise fail), but it diverges from the repo's existing pattern of guarding the import behind if HAS_ASSIMULO: (see tests/integration/reactor_tests.py). Fine to keep; noting for consistency.

Merge readiness

No blocking findings. mergeStateStatus is CLEAN and reviewDecision is empty (no required-review gate reported by branch protection). This COMMENT does not constitute a formal approval, and this account cannot self-approve its own PR; if an approval is required before merge, an eligible maintainer account will need to submit it.

[parkyr]

Adding a quick note to connect this test workaround with the PR body's Related Issues / Priority section.

The Assimulo monkeypatch in this test is temporary glue, not the desired long-term design. Right now, PharmaPy.Reactors imports Assimulo at module import time, so even simple algebra-only unit tests can fail when Assimulo/SUNDIALS is not installed.

That broader problem is already tracked in #10, which proposes lazy solver imports plus a solver interface / SciPy fallback. I think #10 should be prioritized before broad solver-adjacent test expansion or larger unit-operation refactors, because it would remove the need for import-time stubs in core tests.

That said, #10 does not need to block this PR. This PR is a focused correctness fix for #87, and the workaround is only here so the regression test can run under the current package structure.

Related roadmap context: #3 is the broader CI / engineering baseline umbrella, #7 covers test expansion, and #8 covers packaging / dependency cleanup.

[bernalde]

An issue should be created and some commented units should be standarized

[bernalde]

Let’s have this be the chance that if we are adding units in the comments, we switch them to [unit] with the same units than the package pint. I would suggest, let’s open an issue for this but also while adding new code, let’s start pushing for this standard

[bernalde]

Answering to the last comment, the order of the merging would affect the work here. We can merge this now, but once #10 is tackled we would need to come and revisit this code to fix it. Otherwise, we merge the solution to #10 and from there we resolve the merge conflicts here. Do as you wish, but just let me (and GitHub) know