Fix kinetics custom params and equilibrium indexing#111
Conversation
parkyr
left a comment
There was a problem hiding this comment.
Maintainer review — PR #111 (fix for #71)
Reviewed the diff against origin/master merge-base. The PR resolves both defects from the #71 triage artifact: the custom-kinetic_model construction crash in set_params and the array-temp equilibrium reverse-term indexing. The fix is broader than the minimal patch (handles the parameter-estimation round-trip and 2-D deltah_rxn), and the changes are internally consistent.
What I verified
- Correctness of the fix path.
set_paramsdict-branchelsenow assignsparams_f/recordsparams_f_shapeforelem_flag=False; the flat-vectorelserestores it viareshape(self.params_f_shape);concat_paramsravels customparams_f. The round-trip is coherent and theelem_flag=Truepath is untouched. equil_termguards 2-Ddeltah_tempwithinv_temp[:, None] * deltah/gas_ctinstead ofnp.outer(which flattened), andequilibrium_modelnow selectskeq_temp[:, ind]whenkeq_tempis 2-D (arraytemp) while keepingkeq_temp[ind]for the scalar-temp/1-D case. Both indexing cases are correct.- Tests (local, repo
.venv, Python 3.11.15):pytest tests/test_kinetics_custom_and_equilibrium.py -v→ 5 passed. The two equilibrium tests use the trusted scalar/1-D path as an implementation-independent oracle; the flat-update test genuinely exercises theconcat_params↔set_paramsround-trip.pytest tests/ -m "not assimulo"→ 38 passed, 8 deselected.pytest tests/ -m assimulo→ 8 passed (reactor/flowsheet integration; no regression in the touched kinetics paths).git -c core.whitespace=cr-at-eol diff --check→ clean.
- GitHub checks (head
1066d67): Core tests SUCCESS, Assimulo integration tests SUCCESS. Closes #71is the correct link — the PR fully resolves both defects in that issue.
Findings
Nonblocking — Custom-model coverage stops at construction and parameter plumbing; no test drives a user kinetic_model through an actual rate evaluation (e.g. in a reactor integration) to confirm the restored params_f produces correct rates end to end.
- Why it matters: the regression suite proves the feature no longer crashes and that parameters round-trip, but not that the custom rate law is wired correctly downstream.
- Concrete step: this is appropriately scoped for the #71 regression fix (an end-to-end custom-model reactor run needs the assimulo backend and is broader than the defect). Fine to defer; worth a follow-up issue if custom rate laws become a supported/documented path.
Summary
- Blocking: none.
- Nonblocking: 1 (custom-model end-to-end coverage — deferrable).
- Questions: none.
- Tests run: regression (5), non-assimulo suite (38), assimulo integration (8), whitespace check — all green.
- Merge-readiness:
mergeStateStatus: CLEAN, PR-head checks green. No blocking findings.
This account authored the PR, so GitHub does not accept a self-APPROVE; posting as COMMENT. reviewDecision is currently empty (no reported approval gate) — if branch protection later requires an approval, an eligible maintainer will need to provide it.
Summary
RxnKinetics.set_paramsso user-definedkinetic_modelinstances require, store, flatten, and restore customparams_fcorrectly.Keqvalues.Closes #71.
Triage / Acceptance Criteria
Uses the maintainer triage artifact at #71 (comment).
Covered:
RxnKinetics(..., kinetic_model=fn, params_f=arr)constructs and stores the f-parameters.params_fstill raises the intendedRuntimeError.params_fshape and update values.equilibrium_modelmatches scalar per-time evaluation.deltah_rxnfrom heat-of-reaction style inputs no longer goes throughnp.outerflattening.Related Issues
Tests
.venv/bin/python -m pytest tests/test_kinetics_custom_and_equilibrium.py -vfailed on unchanged base with the recorded custom-modelUnboundLocalErrorand vectorized-equilibrium mismatch..venv/bin/python -m pytest tests/test_kinetics_custom_and_equilibrium.py -v.venv/bin/python -m pytest tests/ -m "not assimulo"(38 passed, 8 deselected).venv/bin/python -m pytest --collect-only(46 tests collected)git -c core.whitespace=cr-at-eol diff --checkLocal environment: repository
.venvwith Python 3.11.15. The changed kinetics code is pure NumPy and does not require the assimulo backend.Branch Hygiene
masterfix/issue-71-kinetics-custom-equilibriumorigin/masterat1a3ab3155216f07e9a18c60687a7b6b1c5959202upstream/masterwas fetched at400e1e13ad4ecd3ba202d5fae8c70ec73bdf9c3eand is already an ancestor oforigin/master.PharmaPy/Kinetics.pyortests/test_kinetics_custom_and_equilibrium.py.